Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Federation domain issuer must be https url #2167

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Federation domain issuer must be https url #2167

wants to merge 6 commits into from

Conversation

joshuatcasey
Copy link
Member

Federation domain issuer must be https url

@netlify Netlify
Copy link

netlify bot commented Dec 26, 2024 •
edited
Loading

✅ Deploy Preview for pinniped-dev canceled.

Name Link
🔨 Latest commit 8eb093d
🔍 Latest deploy log https://app.netlify.com/sites/pinniped-dev/deploys/67864e66c9b9e9000857eb4b

@codecov Codecov
Copy link

codecov bot commented Dec 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 31.94%. Comparing base (14e728a) to head (8eb093d).

Files with missing lines Patch % Lines
internal/testutil/kube_server_compatibility.go 0.00% 5 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2167      +/-   ##
==========================================
- Coverage   31.97%   31.94%   -0.03%     
==========================================
  Files         379      379              
  Lines       62113    62084      -29     
==========================================
- Hits        19859    19834      -25     
+ Misses      41724    41720       -4     
  Partials      530      530

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joshuatcasey joshuatcasey force-pushed the jtc/federation-domain-issuer-must-be-https-url branch 6 times, most recently from 594cb8f to c0c88e1 Compare January 2, 2025 17:59
cfryanr
cfryanr reviewed Jan 2, 2025
View reviewed changes
apis/concierge/config/v1alpha1/types_credentialissuer.go.tmpl
// Information needed to form a valid Pinniped-based kubeconfig using this credential issuer.
// This field is deprecated and will be removed in a future version.
// +optional
KubeConfigInfo *CredentialIssuerKubeConfigInfo `json:"kubeConfigInfo,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How confident are you that nobody is using this field? It has been deprecated almost from the start, but did our known clients all stop using it?

Also, did you test what happens on an upgrade when this field disappears from the CRD?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I believe that this was first marked deprecated in v0.7.0.

Found by using git tag --contains 60f92d5fe2f7c5da632b1639e6fd699c0e22d122.

@joshuatcasey
Copy link
Member Author

Just pushed bfbf0c9d03c, tested against the following kind versions:

  • v1.23.17
  • v1.24.17
  • v1.25.16
  • v1.27.16
  • v1.29.12
  • v1.30.8
  • v1.32.0

@joshuatcasey
Copy link
Member Author

Also fixes #468

@joshuatcasey joshuatcasey force-pushed the jtc/federation-domain-issuer-must-be-https-url branch from d2b8101 to d689abc Compare January 7, 2025 17:58
@joshuatcasey joshuatcasey force-pushed the jtc/federation-domain-issuer-must-be-https-url branch from d689abc to 8eb093d Compare January 14, 2025 11:45
@joshuatcasey
Copy link
Member Author

Wait on #2182 to rebase this PR, since CI failed due to a CI flake (https://ci.pinniped.dev/teams/main/pipelines/pull-requests/jobs/build-test-image/builds/200)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfryanr cfryanr cfryanr left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshuatcasey @cfryanr @vmwclabot