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

prowgen: operate on sub-directories #16

Merged
merged 4 commits into from
Jul 12, 2019

Conversation

bbguimaraes
Copy link
Contributor

I decided to give this a time limit of 30m and… it was much easier to implement than I had initially thought. If people are happy with the overall design and implementation, I will work on making it prettier (documentation, comments, etc.).

As discussed in #13 and #14, --from-file is incompatible with our new pruning method. It does have advantages, though, that I think we should maintain. Namely:

  • Speed: it is ~70x faster on my machine, although the difference is seconds vs. centiseconds.
  • Scope: it allows someone working on jobs in a single repository (what I believe is our most common non-expert usage) to focus on their jobs. See prowgen: make --from-file imply --prune=false #14 (comment). Correctness is still enforced by the pre-submit check in openshift/release, which remains unchanged.

The problem with --from-file, AFAICT, is that variants broke the direct mapping we had from ci-operator configuration files to job files. We can no longer safely consider each input file in isolation, which is the basis of our current iteration algorithm.

We can, however, consider a sub-directory in isolation: all variants come from the same leaf directory. This change makes the iteration on both generation and pruning look only at the directories passed as non-flag arguments (previously ignored), similar to git commands, e.g. git diff -- dir0 dir1.

Backward compatibility is maintained by iterating over the entire tree when no argument is given. All tests from #15 pass. I've based this PR on them, the original changes are:

3 files changed, 26 insertions(+), 18 deletions(-)

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 4, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 4, 2019
@petr-muller
Copy link
Member

We can, however, consider a sub-directory in isolation: all variants come from the same leaf directory.

Very nice! I like the approach.

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 4, 2019
@bbguimaraes
Copy link
Contributor Author

So… I decided to read our documentation. What a concept, I know. Actually, I opened the file to remove the references to --from-file. But it turns out we've always supported passing a sub-directory to --from-dir: https://github.com/openshift/ci-tools/blob/6718aa2f9936aa2a55ef2d555c23ba602fbbfef3/CI_OPERATOR_PROWGEN.md#generate-prow-jobs-for-new-ci-operator-config.

I tested it and it works the same as my implementation (unsurprisingly, in retrospect), except for the pruning stage. I will, in that order, fix pruning, remove the extra code for handling non-flag arguments, and never again tell anyone to RTFM.

@bbguimaraes bbguimaraes changed the title prowgen: operate on sub-directories [WIP] prowgen: operate on sub-directories Jul 4, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 4, 2019
return func(configSpec *cioperatorapi.ReleaseBuildConfiguration, info *config.Info) error {
visited.Insert(filepath.Join(info.Org, info.Repo))
Copy link
Member

@petr-muller petr-muller Jul 8, 2019

Choose a reason for hiding this comment

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

This unfortunately does not cover (uncommon but real) corner case when some changes make us to stop visiting a directory entirely, which happens when we remove all config for a given component. This happens when repositories are renamed or archived (I remember both happening). That's why I did not implement pruning like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, I realized that five seconds after pushing, but didn't think people would be monitoring the PR =)

I'll add an integration test for that. I had a hard time yesterday trying to reconcile --from-dir with --prune, in the end I think I'll have to revert to my previous implementation with separate arguments.

I'll see if I can find some time in the next few days, I'm trying not to spend too much time on this.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 11, 2019
@bbguimaraes
Copy link
Contributor Author

  • rebased
  • added a configuration removal test
  • reverted to using separate arguments
  • added an integration test
  • removed --from-file because it cannot work with pruning
  • removed --prune becase pruning should now be safe for all operations
  • removed references to --from-dir path/to/subdir from the documentation

The last two items can be reverted if there are objections.

The reason for reverting to separate arguments is that I could not find a way, with just --from-dir and --to-dir, to determine how to limit pruning without using heuristics while scanning the input directory. That becomes straightforward when the arguments are names of directories under both the input and output directories.

@bbguimaraes bbguimaraes changed the title [WIP] prowgen: operate on sub-directories prowgen: operate on sub-directories Jul 11, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2019
@bbguimaraes
Copy link
Contributor Author

Commit order because Github has one job and fails at it:

$ git log --oneline --reverse master..
b4ee936 prowgen: test complete removal
a5f3f02 prowgen: remove --from-file
99d497c prowgen: remove --prune
605ba96 prowgen: operate on sub-directories

Copy link
Member

@petr-muller petr-muller left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Nice job.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [bbguimaraes,petr-muller]

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

@openshift-merge-robot openshift-merge-robot merged commit e988d10 into openshift:master Jul 12, 2019
@bbguimaraes bbguimaraes deleted the args branch July 12, 2019 15:41
@bbguimaraes
Copy link
Contributor Author

/woof

@openshift-ci-robot
Copy link
Contributor

@bbguimaraes: dog image

In response to this:

/woof

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.

DennisPeriquet pushed a commit to DennisPeriquet/ci-tools that referenced this pull request Jan 6, 2022
# This is the 1st commit message:

force all disruption aggregation to skipped status while we re-gather unbugged data

# This is the commit message openshift#2:

Fix lint issue.

# This is the commit message openshift#3:

sanitize: keep jobs from the same PR on the same cluster

# This is the commit message openshift#4:

Support runIfChanged and skipIfOnlyChanged for postsubmit

Signed-off-by: clyang82 <chuyang@redhat.com>

# This is the commit message openshift#5:

Update UT

Signed-off-by: clyang82 <chuyang@redhat.com>

# This is the commit message openshift#6:

fix integration test

Signed-off-by: clyang82 <chuyang@redhat.com>

# This is the commit message openshift#7:

[DPTP-2635]cluster-display: add endpont for clusters

# This is the commit message openshift#8:

prowgen: stop tolerating changes to `optional` fields

...except for images jobs for which do not have syntax in ci-operator config

Once we pull all manual changes to ci-operator configs, we can stop tolerating these changes. One step closer to fully generated Prowjob configuration.

# This is the commit message openshift#9:

Honor semver when comparing clone target/source

# This is the commit message openshift#10:

Write a README for autoconfigbrancher

# This is the commit message openshift#11:

autoconfigbrancher: simplify a conditional

# This is the commit message openshift#12:

autoconfigbrancher: use a permanent link in README

# This is the commit message openshift#13:

Rename mis-spelled variable, add short for cobra help, add comments

# This is the commit message openshift#14:

add more comments re: GCS

# This is the commit message openshift#15:

Note two tables created in memory using a SELECT

# This is the commit message openshift#16:

fix typo

# This is the commit message openshift#17:

cast to a type so we can see more info on dryrun mode

# This is the commit message openshift#18:

Share ReleaseController's config

# This is the commit message openshift#19:

Add aggregating job results to testgrid (openshift#2576)

* Add aggregating job results to testgrid

* Add aggregating job results to testgrid

Aggregating jobs are added to blocking dashboard of the corresponding release.
Release is determined by the aggregated job prow config.

* Add test cases

* Add nil pointer check
# This is the commit message openshift#20:

Updating auto-include logic for OpenStack jobs

Adding the OpenStack jobs which recently moved from release to
shiftstack directory.
This will add them to testgrid automatically.

# This is the commit message openshift#21:

[DPTP-2660]payload-testing-prow-plugin: format errors

# This is the commit message openshift#22:

handle missing history for aggregated disruption

# This is the commit message openshift#23:

Add create-tables subcommand to create Jobs tab and debugging tips to README

# This is the commit message openshift#24:

create JobRuns table

# This is the commit message openshift#25:

Rename vars to be more generic for table creation

# This is the commit message openshift#26:

add in gofmt fixes for lint

# This is the commit message openshift#27:

comment about Jobs being initially primed

# This is the commit message openshift#28:

comment compile time checks in case go beginners have never seen it

# This is the commit message openshift#29:

prpqr: add a PR deploy makefile target

# This is the commit message openshift#30:

prpqr ui: use centos stream instead of 8 from docker

# This is the commit message openshift#31:

Refuse /payload command on PRs to repositories that do not contribute to OCP

# This is the commit message openshift#32:

allow correcting history without changing test name when we get criteria incorrect

# This is the commit message openshift#33:

switch the check for aggregation passes to use the SQL views
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants