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

🌱 add make target to apply clusterstack resource #93

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

kranurag7
Copy link
Contributor

  • 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

Makefile Outdated Show resolved Hide resolved
config/cspo/clusterstack.yaml Show resolved Hide resolved
@kranurag7 kranurag7 force-pushed the syself/make-targets-for-clusterstack branch from cdf96f2 to 4f761b1 Compare February 20, 2024 19:10
@kranurag7
Copy link
Contributor Author

@chess-knight Can you please take a look at this one? (time permitting on your end)

@chess-knight chess-knight requested review from michal-gubricky and matofeder and removed request for chess-knight February 21, 2024 06:19
Copy link
Contributor

@michal-gubricky michal-gubricky left a 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.

Copy link
Member

@matofeder matofeder left a 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

Tiltfile Outdated Show resolved Hide resolved
@kranurag7 kranurag7 force-pushed the syself/make-targets-for-clusterstack branch from 4f761b1 to fd21347 Compare February 28, 2024 06:07
docs/develop.md Outdated Show resolved Hide resolved
docs/develop.md Outdated Show resolved Hide resolved
config/cspo/clusterstack.yaml Outdated Show resolved Hide resolved
docs/develop.md Outdated Show resolved Hide resolved
@matofeder
Copy link
Member

A rather general comment about the review on DRAFT PR.
IMO the good practice is to review the PR that is not in the DRAFT. For me, the DRAFT means something like WIP, i.e. not ready to review.

@janiskemper
Copy link
Member

@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.
If you don't like it, I would ask @kranurag7 to not do it in this repo in the future.

@matofeder
Copy link
Member

@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. If you don't like it, I would ask @kranurag7 to not do it in this repo in the future.

@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.

Copy link
Member

@matofeder matofeder left a 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!

@janiskemper
Copy link
Member

@matofeder you are completely right about the CI supporting the developers and finding issues. This is why we usually say that make verify should be used regularly to catch obvious mistakes and to kind of "run the CI locally" without anyone getting bothered by the failing checks.
I would expect that @kranurag7 does this in his draft PR as well ;)

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!

@matofeder
Copy link
Member

matofeder commented Feb 28, 2024

@matofeder you are completely right about the CI supporting the developers and finding issues. This is why we usually say that make verify should be used regularly to catch obvious mistakes and to kind of "run the CI locally" without anyone getting bothered by the failing checks. I would expect that @kranurag7 does this in his draft PR as well ;)

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:

  1. IMO CI pipeline could help to have clear code before a reviewer dives into it
  2. What is the expected PR workflow? Something like this?:
    • the reviewer does the review on the draft PR
    • the reviewer approves the PR
    • PR is then marked as ready for review
    • CI pipelines run, (it is expected that without fail (it is expected the developer runs something like make verify locally)
    • PR is merged

@janiskemper
Copy link
Member

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.

@kranurag7 kranurag7 force-pushed the syself/make-targets-for-clusterstack branch from 52f5383 to 7b5421b Compare February 28, 2024 14:42
docs/develop.md Outdated Show resolved Hide resolved
docs/develop.md Outdated Show resolved Hide resolved
Copy link
Member

@janiskemper janiskemper left a 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

Copy link
Contributor

@michal-gubricky michal-gubricky left a 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

@kranurag7 kranurag7 marked this pull request as ready for review February 29, 2024 15:25
@cluster-stack-bot cluster-stack-bot bot added the area/hack Changes made in the hack directory label Feb 29, 2024
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>
@kranurag7 kranurag7 force-pushed the syself/make-targets-for-clusterstack branch from 9a8aa13 to 91e7e50 Compare February 29, 2024 15:41
@cluster-stack-bot cluster-stack-bot bot added the size/M Denotes a PR that changes 50-200 lines, ignoring generated files. label Feb 29, 2024
@kranurag7
Copy link
Contributor Author

Thank you for the reviews, after final testing, I think this is now ready to be merged.

@kranurag7 kranurag7 merged commit 3b8df52 into main Feb 29, 2024
6 checks passed
@kranurag7 kranurag7 deleted the syself/make-targets-for-clusterstack branch February 29, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/hack Changes made in the hack directory size/M Denotes a PR that changes 50-200 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants