-
Notifications
You must be signed in to change notification settings - Fork 19
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
Parameter set controller #101
Conversation
Signed-off-by: Steven Gettys <s.gettys@f5.com>
Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>
Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>
Signed-off-by: Brian DeGeeter <b.degeeter@f5.com>
Codecov Report
@@ Coverage Diff @@
## main #101 +/- ##
==========================================
- Coverage 78.27% 78.07% -0.20%
==========================================
Files 10 12 +2
Lines 833 976 +143
==========================================
+ Hits 652 762 +110
- Misses 115 136 +21
- Partials 66 78 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! I've mostly just flagged some tech debt that we are increasing and want to say that going forward I can't accept more PRs that don't also fix these problems.
//+kubebuilder:rbac:groups=porter.sh,resources=parametersets,verbs=get;list;watch;create;update;patch;delete | ||
//+kubebuilder:rbac:groups=porter.sh,resources=parametersets/status,verbs=get;update;patch | ||
//+kubebuilder:rbac:groups=porter.sh,resources=parametersets/finalizers,verbs=update | ||
//+kubebuilder:rbac:groups=porter.sh,resources=agentconfigs,verbs=get;list;watch;create;update;patch;delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up that I don't think we need to repeat rbac comments from other controllers. I am pretty sure that you could remove everything here but the new rbac lines for parametersets and it wouldn't change any of the generated fields.
Here's the open issue to track cleaning this up but in the meantime we don't need to keep adding new code that has the same problem. 😀 #100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will address with next PR that cleans up all this tech debt!
}, | ||
Spec: porterv1.ParameterSetSpec{ | ||
//TODO: get schema version from porter version | ||
// https://github.com/getporter/porter/pull/2052 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up that this PR is merged, if you'd like to do a follow up PR to address the TODO in these tests. We could bump the version of Porter that we are referencing at the same time.
} | ||
} | ||
|
||
func waitForParamSetDeleted(ctx context.Context, ps *porterv1.ParameterSet) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The amount of copy/paste that is in these test files is really starting to concern me. We are just piling up on code that we know we don't want to maintain like this.
I've created #102 to tack consolidating these test helpers so that 80% of the test files aren't duplicate test helpers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next on the list, will be starting immediately
Signed-off-by: Steven Gettys <s.gettys@f5.com>
7a944f5
to
5a42613
Compare
Signed-off-by: Steven Gettys <s.gettys@f5.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two small fixes and we are good to go 👍
docs/content/file-formats.md
Outdated
The ParameterSet spec is the same schema as the ParameterSet resource in Porter. | ||
You can copy/paste the output of the `porter parameters show NAME -o yaml` command into the ParameterSet resource spec (removing the status section). | ||
|
||
In addition to the normal fields available on a [Porter Parameter Set document](/reference/file-formats/), the following fields are supported: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the link to go directly to the correct section on the page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops copy paste error :)
docs/content/file-formats.md
Outdated
The CredentialSet spec is the same schema as the CredentialSet resource in Porter. | ||
You can copy/paste the output of the `porter credentials show NAME -o yaml` command into the CredentialSet resource spec (removing the status section). | ||
|
||
In addition to the normal fields available on a [Porter Credential Set document](/reference/file-formats/), the following fields are supported: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the link to go directly to the correct section on the page?
Signed-off-by: Steven Gettys <s.gettys@f5.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thank you for implementing parameter and credential sets! 🙇♀️
Added controller for parameter set resource