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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion releng/release-tool/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ go_library(
deps = [
"@com_github_google_go_github_v32//github:go_default_library",
"@com_github_masterminds_semver//:go_default_library",
"@io_k8s_sigs_yaml//:go_default_library",
"@org_golang_x_oauth2//:go_default_library",
],
)
Expand Down Expand Up @@ -49,5 +50,8 @@ go_test(
name = "go_default_test",
srcs = ["release-tool_test.go"],
embed = [":go_default_library"],
deps = ["@com_github_google_go_github_v32//github:go_default_library"],
deps = [
"@com_github_google_go_github_v32//github:go_default_library",
"@io_k8s_sigs_yaml//:go_default_library",
],
)
79 changes: 76 additions & 3 deletions releng/release-tool/release-tool.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ import (
"strings"
"time"

"golang.org/x/oauth2"

"github.com/Masterminds/semver"
"github.com/google/go-github/v32/github"
"golang.org/x/oauth2"
"sigs.k8s.io/yaml"
)

type blockerListCacheEntry struct {
Expand Down Expand Up @@ -414,8 +414,14 @@ func (r *releaseData) forkProwJobs() error {

// create new prow configs if they don't already exist
if _, err := os.Stat(fullOutputConfig); err != nil && os.IsNotExist(err) {
updateJobConfig, err := updatePhase2Jobs(r.org+"/"+r.repo, fullJobConfig)
if err != nil {
return err
}
defer os.Remove(updateJobConfig)

log.Printf("Creating new prow yaml at path %s", fullOutputConfig)
cmd := exec.Command("/usr/bin/config-forker", "--job-config", fullJobConfig, "--version", version, "--output", fullOutputConfig)
cmd := exec.Command("/usr/bin/config-forker", "--job-config", updateJobConfig, "--version", version, "--output", fullOutputConfig)
bytes, err := cmd.CombinedOutput()
if err != nil {
log.Printf("ERROR: config-forker command output: %s : %s ", string(bytes), err)
Expand Down Expand Up @@ -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.

if err != nil {
return "", fmt.Errorf("Error reading yaml file: %v", err)
}

var data map[string]interface{}
if err := yaml.Unmarshal(fileContent, &data); err != nil {
return "", fmt.Errorf("Error unmarshalling yaml: %v", err)
}

err = updatePresubmits(orgRepo, data)
if err != nil {
return "", err
}

updatedContent, err := yaml.Marshal(data)
if err != nil {
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

if err := ioutil.WriteFile(updateJobConfig, updatedContent, 0644); err != nil {
return "", fmt.Errorf("Error writing updated yaml to file: %v", err)
}

return updateJobConfig, nil
}

func updatePresubmits(orgRepo string, data map[string]interface{}) error {
presubmits, ok := data["presubmits"].(map[string]interface{})
if !ok {
return fmt.Errorf("Missing or invalid 'presubmits' section in yaml")
}

repo, ok := presubmits[orgRepo].([]interface{})
if !ok {
return fmt.Errorf("Missing or invalid section for repository '%s' in yaml", orgRepo)
}

for i, job := range repo {
jobMap, ok := job.(map[string]interface{})
if !ok {
return fmt.Errorf("Invalid job format in yaml")
}

if shouldUpdateJob(jobMap) {
jobMap["always_run"] = true
presubmits[orgRepo].([]interface{})[i] = jobMap
}
}

return nil
}

func shouldUpdateJob(jobMap map[string]interface{}) bool {
optional, optionalOk := jobMap["optional"].(bool)
alwaysRun, alwaysRunOk := jobMap["always_run"].(bool)
runIfChanged, runIfChangedOk := jobMap["run_if_changed"].(string)
skipIfOnlyChanged, skipIfOnlyChangedOk := jobMap["skip_if_only_changed"].(string)

return (!alwaysRunOk || !alwaysRun) &&
(!optionalOk || !optional) &&
(!runIfChangedOk || runIfChanged == "") &&
(!skipIfOnlyChangedOk || skipIfOnlyChanged == "")
}
93 changes: 93 additions & 0 deletions releng/release-tool/release-tool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
"reflect"
"testing"
"time"

"github.com/google/go-github/v32/github"
"sigs.k8s.io/yaml"
)

func standardCleanup(r *releaseData) {
Expand Down Expand Up @@ -457,3 +460,93 @@ func TestNewTag(t *testing.T) {
}
}
}

func TestUpdatePhase2Jobs(t *testing.T) {
var (
orgRepo = "kubevirt/kubevirt"
jobConfig string
originalConfig map[string]interface{}
)

originalConfig = map[string]interface{}{
"presubmits": map[string]interface{}{
orgRepo: []interface{}{
map[string]interface{}{
"name": "phase1",
"always_run": true,
},
map[string]interface{}{
"name": "phase2",
"always_run": false,
},
map[string]interface{}{
"name": "optional_job",
"always_run": false,
"optional": true,
},
},
},
}

jobConfig = filepath.Join(os.TempDir(), "job-config.yaml")
err := writeYAMLToFile(originalConfig, jobConfig)
if err != nil {
t.Errorf("writeYAMLToFile failed %s", err)
}

defer os.Remove(jobConfig)

updatedJobConfig, err := updatePhase2Jobs(orgRepo, jobConfig)
if err != nil {
t.Errorf("updatePhase2Jobs failed %s", err)
}

updatedContent, err := ioutil.ReadFile(updatedJobConfig)
if err != nil {
t.Errorf("updatedContent failed %s", err)
}

var updatedData map[string]interface{}
err = yaml.Unmarshal(updatedContent, &updatedData)
if err != nil {
t.Errorf("Unmarshal updatedContent failed %s", err)
}

expectedUpdatedData := map[string]interface{}{
"presubmits": map[string]interface{}{
orgRepo: []interface{}{
map[string]interface{}{
"name": "phase1",
"always_run": true,
},
map[string]interface{}{
"name": "phase2",
"always_run": true,
},
map[string]interface{}{
"name": "optional_job",
"always_run": false,
"optional": true,
},
},
},
}

if !reflect.DeepEqual(expectedUpdatedData, updatedData) {
t.Errorf("updatedData doesn't equal expectedUpdatedData")
}
}

func writeYAMLToFile(data map[string]interface{}, fileName string) error {
yamlContent, err := yaml.Marshal(data)
if err != nil {
return err
}

err = ioutil.WriteFile(fileName, yamlContent, 0644)
if err != nil {
return err
}

return nil
}