-
Notifications
You must be signed in to change notification settings - Fork 62
Release Qualifying Jobs Support (part 1) #711
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 Qualifying Jobs Support (part 1) #711
Conversation
|
/cc @jupierce |
1fd87c4 to
2a3c688
Compare
|
/hold |
1854cea to
01de19f
Compare
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
01de19f to
7b6bebc
Compare
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 added these changes, specifically, to leverage the pre-existing presubmit job, in openshift/release, to validate the new release qualifiers config file.
| { | ||
| name: "Good config with nil qualifier", | ||
| configs: []releasecontroller.ReleaseConfig{{ | ||
| Name: "4.19.0-0.nightly", | ||
| Verify: map[string]releasecontroller.ReleaseVerification{ | ||
| "osd-aws": { | ||
| Optional: true, | ||
| MaxRetries: 2, | ||
| ProwJob: &releasecontroller.ProwJobVerification{ | ||
| Name: "periodic-ci-openshift-release-master-nightly-4.19-osd-aws", | ||
| }, | ||
| Qualifiers: releasequalifiers.ReleaseQualifiers{ | ||
| "rosa": {}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expectedErr: false, | ||
| }, | ||
| { | ||
| name: "Good config with empty qualifier", | ||
| configs: []releasecontroller.ReleaseConfig{{ | ||
| Name: "4.19.0-0.nightly", | ||
| Verify: map[string]releasecontroller.ReleaseVerification{ | ||
| "osd-aws": { | ||
| Optional: true, | ||
| MaxRetries: 2, | ||
| ProwJob: &releasecontroller.ProwJobVerification{ | ||
| Name: "periodic-ci-openshift-release-master-nightly-4.19-osd-aws", | ||
| }, | ||
| Qualifiers: releasequalifiers.ReleaseQualifiers{ | ||
| "rosa": {}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expectedErr: false, | ||
| }, |
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.
These tests are identical
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.
Good catch! I was originally using pointers and just blindly refactored everything when I switched over.
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.
Updated
pkg/releasequalifiers/merge_test.go
Outdated
| }, | ||
| }, | ||
| { | ||
| name: "nil base with notifications", |
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.
| name: "nil base with notifications", | |
| name: "empty base with notifications", |
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.
Same.
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.
Updated
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.
This file seems overly complicated to me. Golang's JSON marshal function already sorts keys for maps, so if we copy ReleaseQualifiers to a map[string]any, all that really needs to be sorted is the slices. Here's another method of pretty-print that passes the unit test. It does a json.Marshal->json.Unmarshal to create a full copy of the original data and then sorts Notifications.Jira.Escalations and Notifications.Slack.Escalations for each qualifier while copying to a generic map[string]any and then running json.MarshalIndent:
// PrettyPrint returns a pretty-printed JSON representation of ReleaseQualifiers
// with alphabetically sorted keys for consistent output
func (rqs ReleaseQualifiers) PrettyPrint() (string, error) {
// use json marshaller to convert all structs to maps
json1, err := json.Marshal(rqs)
if err != nil {
return "", err
}
qualiMap := make(ReleaseQualifiers)
if err := json.Unmarshal(json1, &qualiMap); err != nil {
return "", err
}
genericMap := make(map[string]any)
for id, qualifier := range qualiMap {
// unset labels
qualifier.Labels = nil
// sort escalation slices
if qualifier.Notifications != nil {
if qualifier.Notifications.Jira != nil {
sort.Slice(qualifier.Notifications.Jira.Escalations, func(i, j int) bool {
return qualifier.Notifications.Jira.Escalations[i].Name < qualifier.Notifications.Jira.Escalations[j].Name
})
}
if qualifier.Notifications.Slack != nil {
sort.Slice(qualifier.Notifications.Slack.Escalations, func(i, j int) bool {
return qualifier.Notifications.Slack.Escalations[i].Name < qualifier.Notifications.Slack.Escalations[j].Name
})
}
}
genericMap[string(id)] = qualifier
}
// Marshal with pretty printing
jsonData, err := json.MarshalIndent(genericMap, "", " ")
if err != nil {
return "", err
}
return string(jsonData), nil
}
}
Your function is faster as it doesn't do the extra json conversions, but IMO the above is easier to read/maintain.
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.
The prettyprint stuff was all AI. I just wanted a nice way to read the objects as I was developing.
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.
Updated.
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
/label tide/merge-method-squash |
|
@bradmwilliams: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexNPavel, bradmwilliams 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:
Approvers can indicate their approval by writing |
|
/unhold |
Adding initial support for Release Qualifying Jobs. This introduces the
Qualifiersstanza into theReleaseVerificationdefinitions. It also adds support for specifying the--release-qualifiers-config-pathoption to specify the location of the file holding the supported Release Qualifier definitions.In order for a ReleaseQualifier to be applied to a ReleaseVerificationJob, it must be specified as part of the job definition in the respective ReleaseConfig, like:
Any parameter that are provided will override their corresponding values in the
ReleaseQualifiersConfig(specified via the--release-qualifiers-config-pathoption), like:The
ReleaseQualifiersConfigwill be a YAML file located in theopenshift/releaserepository and contains all the availableQualifiersfor use in the Release Verification Process, like:rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED