-
Notifications
You must be signed in to change notification settings - Fork 43
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
✨ Add command to update olmv1 operator #225
✨ Add command to update olmv1 operator #225
Conversation
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.
Like in #220, I'll hold off on unit tests for now
c08c501
to
2e125d6
Compare
547d3be
to
fb856f8
Compare
fb856f8
to
1af1330
Compare
1af1330
to
b69e0e6
Compare
There was an internal meeting yesterday regarding current CLI efforts in kubectl-operator. The gist of it was:
This is the epic that reflects the roadmap: operator-framework/operator-controller#1424 |
b69e0e6
to
df4c62c
Compare
@camilamacedo86 I'll group some of the responses to your comments to help with the organization at least a little. Multiple Channels (raised in: #225 (comment), #225 (comment), #225 (comment))
Documentation changes/questions (raised in: #225 (comment), #225 (comment), #225 (comment), #225 (comment))
General comments/notes
In the (near) future (operator-framework/operator-controller#1851, operator-framework/operator-controller#1766, operator-framework/operator-controller#1850):
The gist of those future plans is that with the new commands we look to come up with a good base functionality that we can later adjust and extend if we see fit (eg. someone makes a compelling argument why syntax should not follow Side note: sorry for this wall of text with multiple links, but I thought it was beneficial at this point to centralize some topics and provide a common background and reasoning for some of things we are doing now and have been doing in previous commands. |
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.
Hi @azych,
Thank you for your time and explanations!
Regarding: Comment Reference,
I think it's best to discuss each point in its respective thread.
About UX/Flags
For all cases:
- Can we avoid
AND ... <other flag>
? It feels unnecessary—AND is already implicit when multiple flags are used, right? - Can we ensure all flags document their default behavior?
(e.g., forchannels
: if not set, search in all)
PS: I’m fine with improving this later, but a small tweak for clarity wouldn’t hurt. It might also help us during the review process. If we can ensure that, it’d be great! That said, it’s a NIT—I won’t block the PR over it.
Replacing "Operator" with "Cluster Extension"
Let’s leave this out for now.
Final Review Status
I did another pass, and I think the last open point is override-unset-with-current
.
Its usage isn’t entirely clear to me—see comment.
Beyond that, everything else seems addressed or will be handled later.
So unless there’s more on this, Otherwise all fine lgtm
✅
Thanks again for your time and input! 🚀
For some reason I am not able to checkout the PR to test locally using github CLI
May be re-basing it will help |
This is already up to date with the main branch. BTW. Wouldn't you say it would be better to keep issues like out of PR comments? We do have the team slack and IMO it would be best to keep the comments cleaner. |
8261b24
to
b2ccb7e
Compare
I understand your sentiment, my intent is to keep all comment in public domain as much as possible. As we are a community project. I can use use #olm-dev channel for this if you prefer instead of PR comment. Personally I do not see an issue in PR comment vs slack message. |
yes, regardless of whether the project is open- or closed- source, in my opinion issues with arbitrary third party tools (like |
Signed-off-by: Artur Zych <[email protected]>
b2ccb7e
to
c675c45
Compare
we now have full |
} | ||
} | ||
|
||
// nolint: unparam |
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.
Which lint error is this masking?
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.
internal/pkg/v1/action/action_suite_test.go:83:27 unparam withConstraintPolicy
- policy
always receives string(olmv1.UpgradeConstraintPolicyCatalogProvided)
("CatalogProvided"
)
I don't think this needs to be 'fixed', hence nolint
. withConstraintPolicy
is used only to build test operators (extension) in tests. We do have test cases where we check setting a different value, but the base remains the same.
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.
/lgtm
Adds command to update an existing olmv1 operator (
ClusterExtension
).Command has the following signature and flags:
Example walkthrough:
Closes operator-framework/operator-controller#1772