search

SovereignCloudStack / cluster-stack-provider-openstack

Cluster Stack Provider OpenStack
https://scs.community/
Apache License 2.0
2 stars 0 forks source link

:seedling: add make target to apply clusterstack resource #93

Closed kranurag7 closed 8 months ago

kranurag7 commented 9 months ago
kranurag7 commented 9 months ago

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

matofeder commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

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

janiskemper commented 8 months ago

@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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

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