Skip to content
This repository was archived by the owner on Nov 28, 2022. It is now read-only.

[release-v1.2] Different cron times for branches for periodics#1636

Closed
aliok wants to merge 1 commit into
openshift:release-v1.2from
aliok:2022-02-15-update-ci-time-v1_2
Closed

[release-v1.2] Different cron times for branches for periodics#1636
aliok wants to merge 1 commit into
openshift:release-v1.2from
aliok:2022-02-15-update-ci-time-v1_2

Conversation

@aliok
Copy link
Copy Markdown
Member

@aliok aliok commented Feb 15, 2022

/hold

I need to hold this because I first need to test this change in different branches to see if my cron changes work.

Checked:

  • 1.2
  • 1.1
  • 1.0
  • 0.26
  • 0.25 --> script here is outdated. There are many diffs with the openshift/release configuration. I won't touch this script but manually update the config

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2022
@openshift-ci openshift-ci Bot requested review from devguyio and pierDipi February 15, 2022 13:36
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Feb 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok

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 Feb 15, 2022
@aliok
Copy link
Copy Markdown
Member Author

aliok commented Feb 15, 2022

/retest

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Feb 15, 2022

@aliok: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

@aliok
Copy link
Copy Markdown
Member Author

aliok commented Feb 15, 2022

/unhold

All good. The script produces the right things in all branches.

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2022
@aliok
Copy link
Copy Markdown
Member Author

aliok commented Feb 15, 2022

cc @pierDipi

Comment on lines +44 to +56
function generate_cron_expression {
if [[ "$branch" == "knative-v0.25.3" ]]; then
echo '0 1 * * 1-5'
elif [[ "$branch" == "knative-v0.26" ]]; then
echo '0 3 * * 1-5'
elif [[ "$branch" == "knative-v1.0" ]]; then
echo '0 5 * * 1-5'
elif [[ "$branch" == "knative-v1.1" ]]; then
echo '0 7 * * 1-5'
elif [[ "$branch" == "knative-v1.2" ]]; then
echo '0 9 * * 1-5'
fi
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm wondering if we can avoid hard-coding these branches here? 🤔

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.

yeah, I know what you mean. Alternative is maybe create a file in each branch that has the Cron expression. Then we can read the file in the script.

I thought about that but this hardcoding is at least making things more central and I don't have to look at 5 different branches for the cron expression when I want to see when jobs are running.

I am ok with both if you have a strong opinion.

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.

Or, any other ideas?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm fine with starting with this to validate the idea and not spend much effort in crappy shell until it's proven, then move to a more dynamic random distribution using modulo
https://stackoverflow.com/questions/13104478/uniformity-of-random-numbers-taken-modulo-n

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@devguyio
Copy link
Copy Markdown

/assign

}

function print_openshift_47_tests {
cron=$(generate_cron_expression)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
cron=$(generate_cron_expression)
cron="$(generate_cron_expression)"

Copy link
Copy Markdown

@devguyio devguyio left a comment

Choose a reason for hiding this comment

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

Is this just to test on 1.2? we try to create those changes on main then /cherry-pick back to release branches.

@aliok
Copy link
Copy Markdown
Member Author

aliok commented Feb 16, 2022

Is this just to test on 1.2? we try to create those changes on main then /cherry-pick back to release branches.

You cannot run the script on main and that's why I wanted to create the PR on 1.2, in case somebody wants to test the changes.

@aliok
Copy link
Copy Markdown
Member Author

aliok commented Feb 16, 2022

/close

Closing in favor of #1639

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Feb 16, 2022

@aliok: Closed this PR.

Details

In response to this:

/close

Closing in favor of #1639

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.

@openshift-ci openshift-ci Bot closed this Feb 16, 2022
@aliok aliok deleted the 2022-02-15-update-ci-time-v1_2 branch November 30, 2023 09:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants