Skip to content

ci-operator/jobs/openshift/origin: Restore mutable properties for 4.8 and 4.9#19716

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
wking:origin-optional-etc-uniformity
Jul 14, 2021
Merged

ci-operator/jobs/openshift/origin: Restore mutable properties for 4.8 and 4.9#19716
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
wking:origin-optional-etc-uniformity

Conversation

@wking
Copy link
Copy Markdown
Member

@wking wking commented Jun 25, 2021

As discussed in e6b339e (#19513) and similar work, the current branch-forking procedure does not track these job properties which are not covered by ci-operator/config/... entries. This PR attempts to recover some of them for openshift/origin. Generated with a new script (also included in this PR):

$ hack/unify-job-properties.py ci-operator/jobs/openshift/origin
$ make jobs

I haven't gone through and guessed at the mutable properties for which sibling branches disagree, but that shouldn't block us moving ahead to repair where there is agreement.

@openshift-ci openshift-ci Bot requested review from bparees and soltysh June 25, 2021 00:14
@wking wking force-pushed the origin-optional-etc-uniformity branch 2 times, most recently from b9b154f to a79d855 Compare June 25, 2021 00:22
@bparees
Copy link
Copy Markdown
Contributor

bparees commented Jun 30, 2021

it's important we assert some consistency here because when a job is optional/not-always-run in master and required in an older release, what happens is the job bitrots in master, starts permfailing, and then no one can get their backports through the older release.

so i'm a big plus one on:

  1. reconciling the current state at this PR attempts to do
  2. DPTP having better controls to ensure we don't fall out of sync in the future on branches.

What are we doing about #2 going forward?

we currently have a mess in 4.8 because we have a bunch of required/always-run jobs that never pass and seemingly no one noticed because they aren't always-run/required in master (yeah i know, we didn't branch master that long ago, so it may not be the whole cause).

@wking
Copy link
Copy Markdown
Member Author

wking commented Jul 1, 2021

My impression was that #19861 and #19862 were part of the DPTP effort to avoid the need for the sort of repair I'm doing here.

Comment thread hack/unify-job-properties.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get some doc (comments in the code if nothing else?) on what this does/how to invoke it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in a79d855534 -> 4e6afd259e.

@wking wking force-pushed the origin-optional-etc-uniformity branch from a79d855 to 4e6afd2 Compare July 12, 2021 21:18
@bparees
Copy link
Copy Markdown
Contributor

bparees commented Jul 12, 2021

/approve

linter isn't happy

Comment thread hack/unify-job-properties.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does it decide which branch to compare 4.8+4.9 to? (master vs 4.6 vs 4.7?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it only automatically updates if all of 4.6, 4.7, and master agree on the mutable value for that job

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise it just logs the inconsistency in case you want to manually adjust

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it

@wking wking force-pushed the origin-optional-etc-uniformity branch from 4e6afd2 to b68c62b Compare July 13, 2021 22:16
@wking
Copy link
Copy Markdown
Member Author

wking commented Jul 13, 2021

linter isn't happy

I've fixed two of the lint issues with 4e6afd259e -> b68c62ba84. I'm not sure if there's anything I can do about:

hack/unify-job-properties.py:7:0: E0401: Unable to import 'ruamel.yaml' (import-error)

And with that effectively unfixable, I am leaving Redefining name 'directory' from outer scope alone, instead of inventing function-specific alternative names. Checking for precedent, I don't see any comments to make the linter happy around this existing ruamel import, so I expect the linter hiccups here will not cause issues for future PRs touching other files.

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Jul 13, 2021

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2021
@bparees
Copy link
Copy Markdown
Contributor

bparees commented Jul 13, 2021

And with that effectively unfixable, I am leaving Redefining name 'directory' from outer scope alone, instead of inventing function-specific alternative names. Checking for precedent, I don't see any comments to make the linter happy around this existing ruamel import, so I expect the linter hiccups here will not cause issues for future PRs touching other files.

the linter deliberately ignores hack/lib:

so this will still be an issue for subsequent PRs

@bparees
Copy link
Copy Markdown
Contributor

bparees commented Jul 13, 2021

/lgtm cancel

(still need to sort out the linter issue so that all subsequent PRs to this repo don't have a failing linter test to look at)

@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2021
@bparees
Copy link
Copy Markdown
Contributor

bparees commented Jul 13, 2021

(I guess it will only be subsequent PRs that touch .py files, but even so i'd rather fix the linter job now than pile up tech debt for someone to investigate later)

@petr-muller
Copy link
Copy Markdown
Member

/retest-required

wking added 2 commits July 14, 2021 07:27
…operties

As discussed in e6b339e
(ci-operator/jobs/openshift/cluster-version-operator: Metal optional
on 4.8 and 4.9, 2021-07-21, openshift#19513) and similar work, the current
branch-forking procedure does not track these job properties which are
not covered by ci-operator/config/... entries.  This script makes it
easier to mass-repair suspect branches, based on agreement between
master and more reliable sibling branches.
… and 4.9

Generated with:

  $ hack/lib/unify-job-properties.py ci-operator/jobs/openshift/origin
  $ make jobs

I haven't gone through and guessed at the mutable properties for which
sibling branches disagree, but that shouldn't block us moving ahead to
repair where there is agreement.
@wking wking force-pushed the origin-optional-etc-uniformity branch from b68c62b to 916e7c5 Compare July 14, 2021 14:27
@wking
Copy link
Copy Markdown
Member Author

wking commented Jul 14, 2021

the linter deliberately ignores hack/lib....

Rebased onto master and shifted the script under hack/lib/ with b68c62ba84 -> 916e7c5.

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jul 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2021
@openshift-merge-robot openshift-merge-robot merged commit 07e340e into openshift:master Jul 14, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jul 14, 2021

@wking: Updated the job-config-4.8 configmap in namespace ci at cluster app.ci using the following files:

  • key openshift-origin-release-4.8-presubmits.yaml using file ci-operator/jobs/openshift/origin/openshift-origin-release-4.8-presubmits.yaml
Details

In response to this:

As discussed in e6b339e (#19513) and similar work, the current branch-forking procedure does not track these job properties which are not covered by ci-operator/config/... entries. This PR attempts to recover some of them for openshift/origin. Generated with a new script (also included in this PR):

$ hack/unify-job-properties.py ci-operator/jobs/openshift/origin
$ make jobs

I haven't gone through and guessed at the mutable properties for which sibling branches disagree, but that shouldn't block us moving ahead to repair where there is agreement.

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.

@wking wking deleted the origin-optional-etc-uniformity branch July 14, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants