prowgen: add IaaS-agnostic installer test type #6
Conversation
openshift-ci-robot
added
size/L
| @@ -27,6 +27,29 @@ const ( | |||
| sentryDsnSecretName = "sentry-dsn" | |||
| sentryDsnMountPath = "/etc/sentry-dsn" | |||
| sentryDsnSecretPath = "/etc/sentry-dsn/ci-operator" | |||
|
|
|||
| openshiftInstallerRandomCmd = `set -eux | |||
| target=$(awk < /usr/local/e2e-targets \ | |||
There was a problem hiding this comment.
Ugh, awk. If only we had Python available :(
I don't like the fact that we have duplicate lists of platforms (here and in the /usr/local/e2e-targets weights file). I'm thinking we could make /usr/local/e2e-targets a sourceable bash file, covering L32-L41 of this file. Then we would just source it here and it would set template and CLUSTER_TYPE accordingly.
There was a problem hiding this comment.
A couple of points (in decreasing order of importance):
- What's wrong with awk? =)
- This is a quick-and-dirty (actually, the quickest-and-dirtiest) implementation: the goal of the task is to get the jobs in place as soon as possible, then refine. The job is generated by
prowgen, so changing the implementation is trivial. - I treated the format of
e2e-targetsas definitive. If/when this becomes Go code, maybe YAML would make more sense. We can bikeshed the exact format, but I went for "as-simple-as-possible plain text". I think we specially don't want to tie it to a specific implementation, as that is very likely to change. - Isn't it available? I assumed it were, but even then I still think
awkis unbeatable for ad-hoc text processing. Thepythonversion (withrandom.choices, an equivalent implementation would be even longer):
import random, sys
ws, ns = zip(*((float(w), n) for (w, n) in map(str.split, sys.stdin)))
print(random.choices(ns, weights=ws)[0])sorry for the little essay, I wanted to answer other questions that were likely to come up
cmd/ci-operator-prowgen/main.go
Outdated
| @@ -84,7 +107,7 @@ func (o *options) process() error { | |||
| // Generate a PodSpec that runs `ci-operator`, to be used in Presubmit/Postsubmit | |||
| // Various pieces are derived from `org`, `repo`, `branch` and `target`. | |||
| // `additionalArgs` are passed as additional arguments to `ci-operator` | |||
| func generatePodSpec(info *config.Info, target string, additionalArgs ...string) *kubeapi.PodSpec { | |||
| func generatePodSpec(info *config.Info, target string, cmd, args []string, additionalArgs ...string) *kubeapi.PodSpec { | |||
There was a problem hiding this comment.
I don't like how this this makes all default case callsites cluttered with nil, nil. I like the advice in https://dave.cheney.net/practical-go/presentations/qcon-china.html#_design_apis_for_their_default_use_case.
I think the only caller that cares about cmd and args (generatePodSpecRandom) can overwrite the Containers[0].Command and Containers[0].Args fields of the returned podspec, eliminating the impact on other callsites.
There was a problem hiding this comment.
Heh, I spent ~5ms thinking about the interface. Agreed, I will change it to something more decent.
cmd/ci-operator-prowgen/main.go
Outdated
| func generatePodSpecRandom(info *config.Info, test *cioperatorapi.TestStepConfiguration) *kubeapi.PodSpec { | ||
| cmd := []string{"-c", fmt.Sprintf(openshiftInstallerRandomCmd, test.As)} | ||
| podSpec := generatePodSpec(info, test.As, []string{"bash"}, cmd) | ||
| profiles := []string{"aws", "azure4", "vsphere"} |
There was a problem hiding this comment.
I think this should be extracted somewhere more prominent to be more easily found when platforms are added / removed.
There was a problem hiding this comment.
Hmm, I certainly should have used the constants we already have, now that I think about it. I'll move this to a constant.
|
Comments addressed. I am keeping |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bbguimaraes, stevekuznetsov 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 |
# This is the 1st commit message: force all disruption aggregation to skipped status while we re-gather unbugged data # This is the commit message openshift#2: Fix lint issue. # This is the commit message openshift#3: sanitize: keep jobs from the same PR on the same cluster # This is the commit message openshift#4: Support runIfChanged and skipIfOnlyChanged for postsubmit Signed-off-by: clyang82 <chuyang@redhat.com> # This is the commit message openshift#5: Update UT Signed-off-by: clyang82 <chuyang@redhat.com> # This is the commit message openshift#6: fix integration test Signed-off-by: clyang82 <chuyang@redhat.com> # This is the commit message openshift#7: [DPTP-2635]cluster-display: add endpont for clusters # This is the commit message openshift#8: prowgen: stop tolerating changes to `optional` fields ...except for images jobs for which do not have syntax in ci-operator config Once we pull all manual changes to ci-operator configs, we can stop tolerating these changes. One step closer to fully generated Prowjob configuration. # This is the commit message openshift#9: Honor semver when comparing clone target/source # This is the commit message openshift#10: Write a README for autoconfigbrancher # This is the commit message openshift#11: autoconfigbrancher: simplify a conditional # This is the commit message openshift#12: autoconfigbrancher: use a permanent link in README # This is the commit message openshift#13: Rename mis-spelled variable, add short for cobra help, add comments # This is the commit message openshift#14: add more comments re: GCS # This is the commit message openshift#15: Note two tables created in memory using a SELECT # This is the commit message openshift#16: fix typo # This is the commit message openshift#17: cast to a type so we can see more info on dryrun mode # This is the commit message openshift#18: Share ReleaseController's config # This is the commit message openshift#19: Add aggregating job results to testgrid (openshift#2576) * Add aggregating job results to testgrid * Add aggregating job results to testgrid Aggregating jobs are added to blocking dashboard of the corresponding release. Release is determined by the aggregated job prow config. * Add test cases * Add nil pointer check # This is the commit message openshift#20: Updating auto-include logic for OpenStack jobs Adding the OpenStack jobs which recently moved from release to shiftstack directory. This will add them to testgrid automatically. # This is the commit message openshift#21: [DPTP-2660]payload-testing-prow-plugin: format errors # This is the commit message openshift#22: handle missing history for aggregated disruption # This is the commit message openshift#23: Add create-tables subcommand to create Jobs tab and debugging tips to README # This is the commit message openshift#24: create JobRuns table # This is the commit message openshift#25: Rename vars to be more generic for table creation # This is the commit message openshift#26: add in gofmt fixes for lint # This is the commit message openshift#27: comment about Jobs being initially primed # This is the commit message openshift#28: comment compile time checks in case go beginners have never seen it # This is the commit message openshift#29: prpqr: add a PR deploy makefile target # This is the commit message openshift#30: prpqr ui: use centos stream instead of 8 from docker # This is the commit message openshift#31: Refuse /payload command on PRs to repositories that do not contribute to OCP # This is the commit message openshift#32: allow correcting history without changing test name when we get criteria incorrect # This is the commit message openshift#33: switch the check for aggregation passes to use the SQL views
No description provided.