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.

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

feat: initial CredentialSet CRD/Controller #98

Merged
merged 9 commits into from
May 6, 2022

Conversation

bdegeeter
Copy link
Contributor

Provides for basic CredentialSet resource supporting Kubernetes secret as a credential source

@bdegeeter
Copy link
Contributor Author

We'll follow up with a docs update before taking this out of draft.

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>
Co-authored-by: Steven Gettys <steven.gettys@gmail.com>
@sgettys sgettys force-pushed the bdegeeter/issue18 branch from 41aa558 to 8b390e8 Compare April 27, 2022 22:15
@sgettys sgettys requested a review from carolynvs April 27, 2022 22:16
Signed-off-by: Steven Gettys <s.gettys@f5.com>
@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #98 (fde1e24) into main (417031d) will increase coverage by 23.79%.
The diff coverage is 77.08%.

@@             Coverage Diff             @@
##             main      #98       +/-   ##
===========================================
+ Coverage   54.47%   78.27%   +23.79%     
===========================================
  Files           9       10        +1     
  Lines         995      833      -162     
===========================================
+ Hits          542      652      +110     
+ Misses        399      115      -284     
- Partials       54       66       +12     
Flag Coverage Δ
integration-tests ?
unit-tests 78.27% <77.08%> (+23.79%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/v1/credentialset_types.go 66.66% <66.66%> (ø)
controllers/credentialset_controller.go 77.61% <77.61%> (ø)
controllers/porter_resource.go 77.61% <100.00%> (ø)
api/v1/zz_generated.deepcopy.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 417031d...fde1e24. Read the comment docs.

Steven Gettys and others added 3 commits April 27, 2022 16:04
Signed-off-by: Steven Gettys <s.gettys@f5.com>
…port

Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>
Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>
@carolynvs carolynvs self-assigned this Apr 29, 2022
Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>
@bdegeeter bdegeeter marked this pull request as ready for review May 3, 2022 23:44
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

One more that I couldn't comment directly on the file (kubernernetes-linux-amd64) should not be checked in. If you need the plugin binary here, let's grab it using the magefile. Or if you have already done that, this should be in the .gitignore so that it's not accidentally checked in?

.github/workflows/build.yml Show resolved Hide resolved
api/v1/credentialset_types.go Outdated Show resolved Hide resolved
api/v1/credentialset_types.go Outdated Show resolved Hide resolved
api/v1/credentialset_types.go Outdated Show resolved Hide resolved
api/v1/credentialset_types_test.go Outdated Show resolved Hide resolved

var _ = Describe("CredentialSet delete", func() {
Context("when an existing CredentialSet is delete", func() {
It("should run porter credentials delete", func() {
Copy link
Member

Choose a reason for hiding this comment

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

This test seems to be a copy of the update test above, with the extra delete action and versification. One way to make this easier to maintain going forward would be to combine them into a single test that walks through the entire lifecycle of a credential set.

tests/integration/credset_test.go Show resolved Hide resolved
tests/integration/suite_test.go Outdated Show resolved Hide resolved
tests/integration/suite_test.go Outdated Show resolved Hide resolved
tests/integration/testdata/operator_porter_config.yaml Outdated Show resolved Hide resolved
Brian DeGeeter and others added 3 commits May 5, 2022 15:32
Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>
Signed-off-by: Steven Gettys <s.gettys@f5.com>
Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

This looks great! If you can just fix that one test, this is ready to go. 👍

api/v1/credentialset_types_test.go Show resolved Hide resolved
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

oops sorry I see that you do have that else statement, I just wasn't seeing it thanks to the magic of contextual diffs. :shipit:

@carolynvs carolynvs merged commit 337a3f9 into getporter:main May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants