🌱 replace ARG with ENV in builder Dockerfile

[kranurag7]

I think the versions are not getting updated which are backed by ARG. This commit replaces the same with ENV so that we get automatic updates for all the components.

kubectl is excluded because maybe that will be very fast upgrades multiple times a month and kubectl is pretty stable I think and we probably don't need really fast upgrades there.

[kranurag7]

@chess-knight @michal-gubricky I accidentally merged main into this branch. (not an issue, I can fix and force push)

Details


I think this happens because every commit requires to be on top of main branch (only one commit ahead of main)

I think this makes us slow when we want to merge multiple patches in parallel.

Can we please disable this settings if you agree?

[chess-knight]

@chess-knight @michal-gubricky I accidentally merged main into this branch. (not an issue, I can fix and force push)

This is no problem at all IMO because we usually use Squash and merge --> it creates only one commit in the main branch.

I think this happens because every commit requires to be on top of main branch (only one commit ahead of main)

I think this is not true, you can have multiple commits ahead of the main in the pull request, but the feature branch must be up to date(you can rebase+forcepush or merge as you did it) with the target(main) branch

I think this makes us slow when we want to merge multiple patches in parallel.

That's true, you have to at least click the Update branch button and wait for the DCO check, then you do not need to wait for the other checks and you can press Squash and merge immediately(it is not green, but still clickable I think)

Can we please disable this settings if you agree?

This ensures pull requests targeting a matching branch have been tested with the latest code. And you can skip pipelines as I wrote above(If it is not true for you, then raise it again please). So I do not have a strong preference here, what about you @michal-gubricky?

[michal-gubricky]

@chess-knight @michal-gubricky I accidentally merged main into this branch. (not an issue, I can fix and force push)

This is no problem at all IMO because we usually use Squash and merge --> it creates only one commit in the main branch.

I agree with you @chess-knight. I think Squash and merge option is preferable when you merge PR into the main branch as it creates only one commit.

I think this happens because every commit requires to be on top of main branch (only one commit ahead of main)

I think this is not true, you can have multiple commits ahead of the main in the pull request, but the feature branch must be up to date(you can rebase+forcepush or merge as you did it) with the target(main) branch

I think also the reason why this happens is that one PR was merged before this one, so as Roman wrote feature branch was not up-to-date with main branch.

I think this makes us slow when we want to merge multiple patches in parallel.

That's true, you have to at least click the Update branch button and wait for the DCO check, then you do not need to wait for the other checks and you can press Squash and merge immediately(it is not green, but still clickable I think)

Sure, it is slower and you need to go through the process as Roman described. But I think that merging multiple patches in parallel is rare.

Can we please disable this settings if you agree?

This ensures pull requests targeting a matching branch have been tested with the latest code. And you can skip pipelines as I wrote above(If it is not true for you, then raise it again please). So I do not have a strong preference here, what about you @michal-gubricky?

Personally, I would not disable it because it prevents users from merging PR when it is not up-to-date with main branch.

[kranurag7]

Thank you Roman and Michal, I missed squash and merge part.

I think then it's fine and we don't need to do something. I'll stick to current workflow. Thank you for your inputs.

[kranurag7]

@chess-knight I think one downside of squash and merge is that release notes generator is not working nicely with commits merged via squash and merge.

$ go run hack/tools/release/notes.go
Changes since v0.1.0-alpha.3
---

_Thanks to all our contributors!_ 😊

This is required because we use emojis in PRs and want to generate release notes accordingly. I know GitHub gives us a generate-release-notes button while creating releases and we use the same in our previously releases but then we are avoiding benefits of classifying our PRs based on emojis.

By the usage of emojis, it's very clear to distinguish b/w features, bug fixes, breaking changes etc.

// cc @janiskemper

[chess-knight]

@kranurag7 I am wondering why some of the commits in the main branch https://github.com/SovereignCloudStack/cluster-stack-provider-openstack/commits/main/ don't have emojis and some do

[kranurag7]

I don't have an exact answer here but looks like it's also affected by squash and merge probably.

Details


During merge I think emojis are getting missed but this is only based on this PR. On other PRs opened by bot, we don't have this problem looking at commits.

[kranurag7]

It's clear that bot PRs contain emojis in the commit title itself so that's why they don't have missing emojis for bot PRs.

But for this patch We don't have emojis in commit message but it's there on main.

I think we have either added it at the merge time.

[chess-knight]

I think we have Default message configured, see https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests.

The default message uses the commit title and message if the pull request contains only 1 commit, or the pull request title and list of commits if the pull request contains 2 or more commits.

[kranurag7]

Thank you, @janiskemper & me talked about the same yesterday. I think we are reaching a conclusion. We want to keep using emojis.

Now going forward we have two paths:

1. Keep using default message but that will break because commit message generally doesn't contain emojis. If we still want to use that then contributor will have to add :seedling: as prefix to commit messages and remember other emojis as well (:sparkles:) etc.
2. Disable default message and let's go with a more suitable option.

The options are following looking at settings:

Details


[chess-knight]

I am for option 2. Pull request title and commit details and @michal-gubricky you?

[michal-gubricky]

I am for option 2. Pull request title and commit details and @michal-gubricky you?

I also support this option as it eliminates the need for contributors to manually add prefixes to commit messages and to remember all available emojis.

[kranurag7]

Thank you for the review, I'll merge the PR now given and will create an issue referencing this discussion to update the settings in the repo.