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

release-tool: Update phase 2 jobs to be considered phase 1 when forking a new branch #3126

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oshoval
Copy link
Contributor

@oshoval oshoval commented Dec 13, 2023

During new release branch creation, we should convert phase 2 jobs
to be considered phase 1, by setting always_run=true,
because phased plugin is used only for main branch.

Note: we might want to consider #3184 (by removing run_before_merge as well)
but since the jobs become phase 1 it shouldn't affect even if the field remains.

@kubevirt-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 13, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign phoracek for approval. For more information see the Kubernetes Code Review Process.

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

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

@oshoval oshoval changed the title release-tool: Update phase 2 jobs to be consider phase 1 when forking a new branch release-tool: Update phase 2 jobs to be considered phase 1 when forking a new branch Dec 13, 2023
@oshoval oshoval closed this Dec 13, 2023
@oshoval oshoval reopened this Dec 14, 2023
return "", fmt.Errorf("Error marshalling yaml: %v", err)
}

updateJobConfig := os.TempDir() + "/job-config.yaml"
Copy link
Contributor Author

@oshoval oshoval Dec 14, 2023

Choose a reason for hiding this comment

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

should we use rand name ? just if we need to allow reentrancy

@oshoval oshoval marked this pull request as ready for review December 24, 2023 08:40
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 24, 2023
…ng a new branch

Signed-off-by: Or Shoval <oshoval@redhat.com>
@oshoval
Copy link
Contributor Author

oshoval commented Jan 15, 2024

/cc @brianmcarey @dhiller
needed once we start working with phased plugin

@@ -1271,3 +1277,70 @@ func main() {
}
}
}

func updatePhase2Jobs(orgRepo, fullJobConfig string) (string, error) {
fileContent, err := ioutil.ReadFile(fullJobConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason why you don't use config.ReadJobConfig from kubernetes/test-infra? I think that could simplify the code, i.e. making unnecessary all the type conversion?

Usage example:

jobConfig, err := config.ReadJobConfig(requirePresubmitsOpts.jobConfigPathKubevirtPresubmits)

Copy link
Contributor Author

@oshoval oshoval Mar 5, 2024

Choose a reason for hiding this comment

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

i think that when i tried something alike it reordered the fields and did a mass
i can try to make sure with this function

EDIT - here we can see the previous try, i will check if that function uses the one you suggested
(or just try with yours)
https://github.com/kubevirt/project-infra/compare/c42e6a20402114f19a49a528539ff35c1359e6b8..ac8a5d6a7e206ff8c35ce4e03c7b0cf4a75e8980#diff-585313f6146df52ade10307f2f7bd15a57cddd496439c6cc2d8c1cdd30eb7562L1290

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's the func I meant. I don't know if it matters much that it reorders the fields, since this is from perspective of the forked jobs, which aren't yet committed anyway, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't remember how i tested it exactly,
when i test it now it totally mess the file (did some tricks there to tests it quick and dirty)
IIRC, at the past it was either messing it or added tons of nil fields and this was the reason i dropped it
The current solution is the cleanest i found tbh in matter of the output and also the code is pretty straight forward and has unit tests.

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Mar 8, 2024

@oshoval: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-prow-kubevirt-labels-update-precheck 45b45d6 link true /test pull-prow-kubevirt-labels-update-precheck
pull-prow-nmstate-labels-update-precheck 45b45d6 link true /test pull-prow-nmstate-labels-update-precheck

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-sigs/prow repository. I understand the commands that are listed here.

@oshoval
Copy link
Contributor Author

oshoval commented Apr 7, 2024

@brianmcarey @dhiller
Wdyt about the following idea:
Close this PR and even without phased plugin supporting branches, it will still work and just run the phase 2
by tide due to "run_before_merge".
It is totally safe as long as we use "run_before_merge" flag.
If in the future we fix phased plugin such that this flag can be removed, then we will need to revisit this PR or some other solutions (like supporting phased plugin for branches etc)
Alternative is to continue working on the PR / take as it.

Note: since we don't use atm FromBranchProtection, the lanes basically are not considered required for entering the merge queue, but they are considered required for Tide when it creates batch / merge candids due to "run_before_merge".
This why it won't block the PR to enter the merge queue, and on the other hand would run before merging.

@oshoval
Copy link
Contributor Author

oshoval commented Apr 7, 2024

i am closing this PR, i think that everything will work atm out of the box, (no need to change nor the always run nor the run_before_merge)
please see previous comment explanation
if someone is against please let me know

Thanks

/close

@kubevirt-bot
Copy link
Contributor

@oshoval: Closed this PR.

In response to this:

i am closing this PR, i think that everything will work atm out of the box, (no need to change nor the always run nor the run_before_merge)
please see previous comment explanation
if someone is against please let me know

Thanks

/close

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.

@oshoval
Copy link
Contributor Author

oshoval commented Jul 3, 2024

/reopen

@kubevirt-bot kubevirt-bot reopened this Jul 3, 2024
@kubevirt-bot
Copy link
Contributor

@oshoval: Reopened this PR.

In response to this:

/reopen

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-sigs/prow repository.

@oshoval
Copy link
Contributor Author

oshoval commented Jul 3, 2024

reopened upon Brian request
It should solve new release creation
imo it is good as is, hope you will agree

@brianmcarey
Copy link
Member

reopened upon Brian request It should solve new release creation imo it is good as is, hope you will agree

thanks @oshoval - this is an option for fixing - #3478

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants