-
Notifications
You must be signed in to change notification settings - Fork 140
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
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
da827f9
to
1776cdc
Compare
return "", fmt.Errorf("Error marshalling yaml: %v", err) | ||
} | ||
|
||
updateJobConfig := os.TempDir() + "/job-config.yaml" |
There was a problem hiding this comment.
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
96aee65
to
4e687a0
Compare
7a57108
to
050d731
Compare
…ng a new branch Signed-off-by: Or Shoval <[email protected]>
/cc @brianmcarey @dhiller |
@@ -1271,3 +1277,70 @@ func main() { | |||
} | |||
} | |||
} | |||
|
|||
func updatePhase2Jobs(orgRepo, fullJobConfig string) (string, error) { | |||
fileContent, err := ioutil.ReadFile(fullJobConfig) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@brianmcarey @dhiller Note: since we don't use atm |
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) Thanks /close |
@oshoval: Closed this PR. In response to this:
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. |
/reopen |
@oshoval: Reopened this PR. In response to this:
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. |
reopened upon Brian request |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with /lifecycle rotten |
@xpivarc this is the PR I talked about in today's sig-ci meeting |
/remove-lifecycle rotten |
PR needs rebase. 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: The following tests failed, say
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. |
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.