Skip to content

Allow users to generate periodic jobs#60

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
stevekuznetsov:skuznets/generate-periodics
Oct 2, 2019
Merged

Allow users to generate periodic jobs#60
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
stevekuznetsov:skuznets/generate-periodics

Conversation

@stevekuznetsov
Copy link
Copy Markdown
Contributor

In addition to generating presubmits and postsubmits, a new field on the
test type allows users to generate periodic jobs. In order to support
generating periodic jobs that require the src-build step, we add the
repo under test as the first extra_ref. Jobs that do not need to clone
anything will no-op.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 7, 2019
@stevekuznetsov stevekuznetsov force-pushed the skuznets/generate-periodics branch from d6f3bac to 2cdbf14 Compare August 8, 2019 02:23
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2019
@stevekuznetsov stevekuznetsov force-pushed the skuznets/generate-periodics branch from 2cdbf14 to 1ec8316 Compare August 8, 2019 18:57
@stevekuznetsov
Copy link
Copy Markdown
Contributor Author

@droslean @petr-muller @bbguimaraes PTAL

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2019
@petr-muller
Copy link
Copy Markdown
Member

@stevekuznetsov I'll look tomorrow, but one thing that is surprising to me is that breaking-changes did not fail. Until now AFAIK prowgen (and determinizer) did not touch the files with periodics, which means that those jobs are are unorderded, sometimes contain falsy values that would not be serialized etc. I would expect enabling periodics with prowgen to reorder all of them.

@stevekuznetsov
Copy link
Copy Markdown
Contributor Author

Very interesting. I'll look again.

@droslean
Copy link
Copy Markdown
Member

droslean commented Aug 8, 2019

Breaking-changes didn't fail, because no one is using the new feature in the existing ci-operator configs... Therefore, no new periodic jobs have been created, so no changes.

@stevekuznetsov
Copy link
Copy Markdown
Contributor Author

From a quick skim, the determinizer does re-write the periodics, so I think Nikos is right here.

@droslean
Copy link
Copy Markdown
Member

droslean commented Aug 8, 2019

/lgtm
/hold

@petr-muller If you wanna take a look.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Aug 8, 2019
@petr-muller
Copy link
Copy Markdown
Member

From a quick skim, the determinizer does re-write the periodics, so I think Nikos is right here.

I don't think so - the determinizer only does ReadFromDir and then WriteToDir and both of those only operate on Presubmits and Postsubmits...

See e.g. here, the keys are not sorted.

@droslean Please read what I wrote - I'm not expecting new jobs to be generated, but existing, non-generated periodics to be "determinized".

@petr-muller
Copy link
Copy Markdown
Member

petr-muller commented Aug 9, 2019

Ah, pruning:

if info.Type == "periodics" {

This explains why breaking-changes did not fail - because we continue to not touch periodics during pruning. This also causes the generated job to have ci-operator.openshift.io/prowgen-controlled: newly-generated label (newly-generated is the temporary value which pruning uses to recognize jobs that are not stale). So this should be fixed.

A good test would also be trying to generate a periodic for a component which already has some handcrafted periodics (like openshift/release) and checking if these handcrafted jobs are kept, determinized, and new ones correctly added.

ReadFromDir needs to be extended, too (that would make determinizer handle periodics)

Copy link
Copy Markdown
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.

Places that should taught about periodics:

@stevekuznetsov stevekuznetsov force-pushed the skuznets/generate-periodics branch from 1ec8316 to 4ba611a Compare August 9, 2019 15:40
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2019
@stevekuznetsov stevekuznetsov force-pushed the skuznets/generate-periodics branch 2 times, most recently from 324801d to 0d62df3 Compare August 9, 2019 18:01
@stevekuznetsov stevekuznetsov force-pushed the skuznets/generate-periodics branch from 0d62df3 to 725a64b Compare August 30, 2019 15:15
In addition to generating presubmits and postsubmits, a new field on the
test type allows users to generate periodic jobs. In order to support
generating periodic jobs that require the `src-build` step, we add the
repo under test as the first `extra_ref`. Jobs that do not need to clone
anything will no-op.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
Copy link
Copy Markdown
Contributor Author

/test breaking-changes

@hongkailiu
Copy link
Copy Markdown
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: droslean, hongkailiu, stevekuznetsov

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:
  • OWNERS [droslean,hongkailiu,stevekuznetsov]

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

@stevekuznetsov
Copy link
Copy Markdown
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2019
@openshift-merge-robot openshift-merge-robot merged commit d697bd5 into openshift:master Oct 2, 2019
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.

6 participants