Before branching day: step 1 #2815
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Nice, but I don't think that the folder structure should be designed like that. Just put each tool into his own folder in ci-tools/cmd. Also, none of the tools should execute something from an external Makefile since all logic already exists programmatically.
| @@ -0,0 +1,11 @@ | |||
| build-all: | |||
There was a problem hiding this comment.
I don't think that the Makefile is necessary
| - [RPM dependencies mirroring services](cmd/rpm-deps-mirroring-services/README.md) | ||
| - [Generated release gating jobs](cmd/generated-release-gating-jobs/README.md) | ||
|
|
||
| ## HowTo |
| flag.StringVar(&o.releaseJobsDir, "release-jobs", releaseJobsPath, "Path to release-gating jobs .yaml files") | ||
| flag.StringVar(&o.releaseProjDir, "release-proj", "", "Path to 'openshift/release/ folder") | ||
| flag.StringVar(&o.outDir, "out", "", "Output directory. Default to --release-jobs dir") | ||
| flag.StringVar(&o.lifecycleConfigFilePath, "lifecycle-config", lifecycleConfigFilePathDefaultPath, "Path to the lifecycle config file") |
There was a problem hiding this comment.
since you are doing that, can you generalize this flag so we can bind it in the rest of the tools as well?
| } | ||
| } | ||
|
|
||
| if err = utilerrors.NewAggregate(errs); err != nil { |
There was a problem hiding this comment.
| if err = utilerrors.NewAggregate(errs); err != nil { | |
| if err := utilerrors.NewAggregate(errs); err != nil { |
| - GOOD: `/my/nice/absolute/path/to/config.yaml` | ||
| - ALSO-GOOD: `/my/nice/absolute/path/to/a/dir` | ||
|
|
||
| ## Exit codes |
There was a problem hiding this comment.
I don't think that we care about having different kinds of exit codes. Just logging the error is enough.
...ranchingconfigmanagers/release-jobs-config-manager/cmd/generated-release-gating-jobs/main.go
Outdated
Show resolved
Hide resolved
| func gatherOptions() (*options, error) { | ||
| var errs []error | ||
| o := &options{} | ||
| flag.StringVar(&o.curOCPVersion, "cur-ocp-ver", "", "Current OCP version") |
There was a problem hiding this comment.
| flag.StringVar(&o.curOCPVersion, "cur-ocp-ver", "", "Current OCP version") | |
| flag.StringVar(&o.curOCPVersion, "current-release", "", "Current OCP version") |
| var errs []error | ||
| o := &options{} | ||
| flag.StringVar(&o.curOCPVersion, "cur-ocp-ver", "", "Current OCP version") | ||
| flag.StringVar(&o.futureOCPVersion, "future-ocp-ver", "", "Next OCP version") |
There was a problem hiding this comment.
| flag.StringVar(&o.futureOCPVersion, "future-ocp-ver", "", "Next OCP version") | |
| flag.StringVar(&o.futureOCPVersion, "future-release", "", "Next OCP version") |
| o := &options{} | ||
| flag.StringVar(&o.curOCPVersion, "cur-ocp-ver", "", "Current OCP version") | ||
| flag.StringVar(&o.futureOCPVersion, "future-ocp-ver", "", "Next OCP version") | ||
| flag.StringVar(&o.releaseProjDir, "release-proj", "", "Path to 'openshift/release/ folder") |
There was a problem hiding this comment.
| flag.StringVar(&o.releaseProjDir, "release-proj", "", "Path to 'openshift/release/ folder") | |
| flag.StringVar(&o.releaseProjDir, "release-repo-path", "", "Path to openshift/release repository folder") |
| logrus.Debugf("using options %+v", o) | ||
|
|
||
| recStatus, err := reconcile(time.Now(), o) | ||
| switch recStatus { |
There was a problem hiding this comment.
There is no need to check different error codes. Just output the error is enough.
There was a problem hiding this comment.
So do we only go on iff everything went ok (exitCode == ReconcileOK)?
There was a problem hiding this comment.
If you want the program to exit on that specific error, you can use logrus.WIthError(err).Fatal("...."). Fatal will terminate the code with a non-zero return code.
| @@ -0,0 +1,127 @@ | |||
| package bumper | |||
There was a problem hiding this comment.
There is no need for the whole package. Please check: https://github.com/openshift/ci-tools/blob/master/pkg/config/load.go#L242
cmd/branchingconfigmanagers/release-jobs-config-manager/internal/bumper/default-bumper.go
Outdated
Show resolved
Hide resolved
danilo-gemoli
force-pushed
the
feature/auto-br-cuts-release-jobs
branch
from
bdfb19b
to
b8ba3db
Compare
danilo-gemoli
force-pushed
the
feature/auto-br-cuts-release-jobs
branch
3 times, most recently
from
0b8d666
to
0675b70
Compare
danilo-gemoli
force-pushed
the
feature/auto-br-cuts-release-jobs
branch
4 times, most recently
from
e8e2aa3
to
043f645
Compare
danilo-gemoli
force-pushed
the
feature/auto-br-cuts-release-jobs
branch
2 times, most recently
from
4c89fa2
to
3b8c1fa
Compare
danilo-gemoli
force-pushed
the
feature/auto-br-cuts-release-jobs
branch
from
3b8c1fa
to
3c281bd
Compare
danilo-gemoli
force-pushed
the
feature/auto-br-cuts-release-jobs
branch
from
3c281bd
to
071f66c
Compare
|
@danilo-gemoli what about this one? |
openshift-merge-robot
added
the
needs-rebase
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
danilo-gemoli
force-pushed
the
feature/auto-br-cuts-release-jobs
branch
from
071f66c
to
a4c4d58
Compare
|
/retitle Before branching day: step 1 |
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
removed
the
lifecycle/stale
openshift-ci
bot
changed the title
openshift-ci
bot
added
approved
danilo-gemoli
force-pushed
the
feature/auto-br-cuts-release-jobs
branch
from
a4c4d58
to
fb0b806
Compare
| ### Example | ||
| ```sh | ||
| $ ./generated-release-gating-jobs \ | ||
| --current-release "4.12" \ |
There was a problem hiding this comment.
I thought about it and it actually can stay that way. This tool is supporting only one action so there is absolutely no need for it to parse (outdated) lifecycle file
| ini.PrettyFormat = false | ||
| ini.PrettyEqual = true | ||
|
|
||
| // What follow should have be avoided by using f.SaveTo(path) directly, |
There was a problem hiding this comment.
I do not 100% get the explanation, also not sure if we should have such a detailed explanation here. What would be desired way to handle this issue?
There was a problem hiding this comment.
The library prints an annoying double '\n' at the end of the file. I fixed this little glitch some months ago: go-ini/ini/pull/321 but we are still referencing the old version.
| @@ -0,0 +1,110 @@ | |||
| package bumper | |||
There was a problem hiding this comment.
Are these bumper files used in the second PR?
There was a problem hiding this comment.
Yes, that's why I have created this abstraction. It will become more obvious when you will be reviewing the second and third PRs.
| @@ -0,0 +1,110 @@ | |||
| package bumper | |||
There was a problem hiding this comment.
This one will be skipped (and I think it was skipped) by your bumper logic: https://github.com/openshift/release/blob/master/ci-operator/config/openshift/release/openshift-release-master__nightly-4.13.yaml#L560
There was a problem hiding this comment.
Yes, you are correct. I know those tools could potentially miss some other sections as well, especially when it comes to handle non-structured text. As long as the field that it is going to be modified is well defined and it has a structure it is quite easy to perform the replacement. In all other cases I simply do not have strategies other than a naive text/regex replacement and that is dangerous because you could end up modifying text that it is not supposed to be modified.
Example:
Suppose you want to replace release-4.13 with release-4.14 using the sed expression s/4\.13/4\.14/g inside the following script:
curl -L https://github.com/openshift-cnv/cnv-ci/tarball/release-4.13 -o /tmp/cnv-ci.tgz
# Do some work here
# ...
# I'm going to do this and that because of a bug since version 4.13
# ...That would be transformed into:
- curl -L https://github.com/openshift-cnv/cnv-ci/tarball/release-4.13 -o /tmp/cnv-ci.tgz
+ curl -L https://github.com/openshift-cnv/cnv-ci/tarball/release-4.14 -o /tmp/cnv-ci.tgz
# Do some work here
# ...
- # I'm going to do this and that because of a bug since version 4.13
+ # I'm going to do this and that because of a bug since version 4.14
# ...While the first replacement is probably good, the second one for sure it is not as we have changed the semantic of the original text.
Of course we could decide to set some rules as "skip comments if it is a bash script" but I have realized you won't never be 100% sure you made the correct decision if we go down this path.
I bet this is a problem that even @petr-muller faced again and again.
I am obviously open to any advises.
There was a problem hiding this comment.
TBH I never worried too much about substitutions like this (which likely resulted in some wrong results but we were always on that fine line between process efficiency and correctness). I think the only place where we did sed substitution was bumping relesae payload jobs, and that was a basically a black box incantation that nobody eventually understood after a few bumps :)
That said, I think a reasonable approach is to not have ambition to bump versions in unstructured texts. You could have a contract that some part of the CI system exposes the "bumped" values in e.g. an envvar, and if people's payloads want to be handled by the tooling, they should use the nevvar. If they don't, they won't be bumped.
There was a problem hiding this comment.
and I agree. You should either follow the "contract" we established beforehand or those files (or at least some of their stanzas/sections/fields/whatever) won't be bumped to the next version.
That's it, unless we don't want to start training a neural network... that won't be 100% accurate either! :)
There was a problem hiding this comment.
+ # I'm going to do this and that because of a bug since version 4.14
Most of these problems will around curl as I noticed in the past so I would definitely welcome some regexp that is taking care at least of this case.
which likely resulted in some wrong results but we were always on that fine line between process efficiency and correctness
Generally, I would say yes and agree, but on the other hand, as we are not that experienced still, we would like to avoid possible situations when something is not working because we put the accent on efficiency instead of correctness. I also feel now more eyes are watching the process and there is a bigger pressure to do it right.
@danilo-gemoli with that being said, I am ok to merge it as an initial version that could be corrected in the future.
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, jmguzik The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@danilo-gemoli: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
This is a tool that tries to automate the first step of phase "Before branching day" .
Check Centralized Release Branching and Config Management V3
/cc @jmguzik @droslean