-
Notifications
You must be signed in to change notification settings - Fork 0
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 make target to apply clusterstack resource #93
Conversation
cdf96f2
to
4f761b1
Compare
@chess-knight Can you please take a look at this one? (time permitting on your end) |
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.
Based on these changes, we should also modify the docs a little bit.
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.
+1 to @michal-gubricky comment about docs
, besides this, just a few minor comments
4f761b1
to
fd21347
Compare
A rather general comment about the review on DRAFT PR. |
@matofeder the reason why we follow the approach with draft PRs in all our repos is to not burden the CI so much. In open-source repos it is free, but the CI often cannot run in parallel, for example. This is not relevant in all repos and AFAIK also not relevant in this repo, but we introduced this rule to not confuse people too much. |
@janiskemper thank you for the explanation! IMO in free repos, the CI pipeline could help to have clear code before a reviewer dives into it (e.g. code-linters could find some issues), but now I also understand why you prefer the approach with draft PRs. I do not want to force anything here, so let the developer decide. I was just curious about that. |
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!
@kranurag7 thank you!
@matofeder you are completely right about the CI supporting the developers and finding issues. This is why we usually say that But I think this is a valuable discussion and we should probably have a common understanding among all teams and across all Go-related repos about this! We should maybe bring it to the container team meeting! |
yep let's discuss this in the larger round, Here are my two cents:
|
that's pretty much how we do it. We choose this workflow, because we didn't integrate an elaborate chatops like prow, that allows commands like "ok-to-test" etc. |
52f5383
to
7b5421b
Compare
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 from my side - didn't test it out though
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, I also tested it and didn't encounter any issues
this commit adds make target related to clusterstacks to the repo. We want to let tilt start the controller first and then apply the clusterstack and openstackclusterstackreleasetemplate resource via a click on tilt UI. In creation of resources, we are creating openstackclusterstackreleasetemplate first followed by clusterstack resource. Signed-off-by: kranurag7 <anurag.kumar@syself.com> Co-authored-by: Matej Feder <matej.feder@dnation.cloud> Signed-off-by: kranurag7 <81210977+kranurag7@users.noreply.github.com>
9a8aa13
to
91e7e50
Compare
Thank you for the reviews, after final testing, I think this is now ready to be merged. |
add make target to apply clusterstack resource
this commit adds make target related to
clusterstacks to the repo. We want to let tilt
start the controller first and then apply the
clusterstack and openstackclusterstackreleasetemplate
resource via a click on tilt UI.
In creation of resources, we are creating
openstackclusterstackreleasetemplate first followed
by clusterstack resource.
Signed-off-by: kranurag7 anurag.kumar@syself.com