Skip to content
This repository was archived by the owner on Jun 14, 2019. It is now read-only.

Handle sharded ci-operator CMs in rehearse #104

Merged
openshift-merge-robot merged 2 commits into from
Mar 8, 2019
Merged

Handle sharded ci-operator CMs in rehearse #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86fa2c5
Refresh pj-rehearse integration test data
petr-muller Mar 7, 2019
ef7ae54
Handle sharded ci-operator CMs in rehearse
petr-muller Mar 7, 2019
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: 6 additions & 0 deletions pkg/config/load.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"path"
"path/filepath"
"regexp"
"strings"

"github.com/ghodss/yaml"
Expand Down Expand Up @@ -58,6 +59,11 @@ func (i *Info) ConfigMapName() string {
return fmt.Sprintf("ci-operator-%s-configs", promotion.FlavorForBranch(i.Branch))
}

// IsCiopConfigCM returns true if a given name is a valid ci-operator config ConfigMap
func IsCiopConfigCM(name string) bool {
return regexp.MustCompile(`^ci-operator-.+-configs$`).MatchString(name)
}

// We use the directory/file naming convention to encode useful information
// about component repository information.
// The convention for ci-operator config files in this repo:
Expand Down
7 changes: 6 additions & 1 deletion pkg/config/load_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,14 @@ func TestInfo_ConfigMapName(t *testing.T) {
for _, testCase := range testCases {
t.Run(testCase.expected, func(t *testing.T) {
info := Info{Branch: testCase.branch}
if actual, expected := info.ConfigMapName(), testCase.expected; !reflect.DeepEqual(actual, expected) {
actual, expected := info.ConfigMapName(), testCase.expected
if !reflect.DeepEqual(actual, expected) {
t.Errorf("%s: didn't get correct basename: %v", testCase.name, diff.ObjectReflectDiff(actual, expected))
}
// test that ConfigMapName() stays in sync with IsCiopConfigCM()
if !IsCiopConfigCM(actual) {
t.Errorf("%s: IsCiopConfigCM() returned false for %s", testCase.name, actual)
}
})
}
}
2 changes: 0 additions & 2 deletions pkg/config/release.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ const (
CiopConfigInRepoPath = "ci-operator/config"
// TemplatesPath is the path of the templates from release repo
TemplatesPath = "ci-operator/templates"
// Name of the configmap that stores all ci-operator configs
CiOperatorConfigsCMName = "ci-operator-configs"
)

// ReleaseRepoConfig contains all configuration present in release repo (usually openshift/release)
Expand Down
2 changes: 1 addition & 1 deletion pkg/diffs/diffs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func GetPresubmitsForCiopConfigs(prowConfig *prowconfig.Config, ciopConfigs conf
if env.ValueFrom.ConfigMapKeyRef == nil {
continue
}
if env.ValueFrom.ConfigMapKeyRef.Name == config.CiOperatorConfigsCMName {
if config.IsCiopConfigCM(env.ValueFrom.ConfigMapKeyRef.Name) {
Copy link
Contributor

@stevekuznetsov stevekuznetsov Mar 7, 2019

Choose a reason for hiding this comment

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

Wait so this method just returns any presubmit that uses ci-op in a configmap mount? How's it used?

Copy link
Member Author

@petr-muller petr-muller Mar 7, 2019

Choose a reason for hiding this comment

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

The method returns any presubmit that references one of the configs in ciopConfigs. The rehearsal main() uses this method to obtain a set of jobs that were affected by a change to a ciop config - it passes just the ciop configs that were changed into this method.

Copy link
Contributor

@stevekuznetsov stevekuznetsov Mar 7, 2019

Choose a reason for hiding this comment

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

How is it doing the filtering for things actually in the ciopConfigs? Right now it returns any presubmit that mounts any CI Op config file?

Copy link
Member Author

@petr-muller petr-muller Mar 8, 2019

Choose a reason for hiding this comment

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

// if it is one of the CMs with ciop configs
if config.IsCiopConfigCM(env.ValueFrom.ConfigMapKeyRef.Name) {  
  // if the key into the CM is one of the filenames in `ciopConfigs`
  if _, ok := ciopConfigs[env.ValueFrom.ConfigMapKeyRef.Key]; ok {
    ret.Add(repo, job)
  }
}

if _, ok := ciopConfigs[env.ValueFrom.ConfigMapKeyRef.Key]; ok {
ret.Add(repo, job)
}
Expand Down
17 changes: 12 additions & 5 deletions pkg/diffs/diffs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,13 @@ func makeBaseTemplates(baseParameters []templateapi.Parameter, baseObjects []run
}

func TestGetPresubmitsForCiopConfigs(t *testing.T) {
baseCiopConfig := config.Info{
Org: "org",
Repo: "repo",
Branch: "branch",
Filename: "org-repo-branch.yaml",
}

basePresubmitWithCiop := prowconfig.Presubmit{
JobBase: prowconfig.JobBase{
Agent: string(pjapi.KubernetesAgent),
Expand All @@ -408,7 +415,7 @@ func TestGetPresubmitsForCiopConfigs(t *testing.T) {
ValueFrom: &v1.EnvVarSource{
ConfigMapKeyRef: &v1.ConfigMapKeySelector{
LocalObjectReference: v1.LocalObjectReference{
Name: config.CiOperatorConfigsCMName,
Name: baseCiopConfig.ConfigMapName(),
},
},
},
Expand All @@ -433,19 +440,19 @@ func TestGetPresubmitsForCiopConfigs(t *testing.T) {
ret := prowconfig.Presubmit{}
deepcopy.Copy(&ret, &basePresubmitWithCiop)
ret.Name = "job-for-org-repo"
ret.Spec.Containers[0].Env[0].ValueFrom.ConfigMapKeyRef.Key = "org-repo-branch.yaml"
ret.Spec.Containers[0].Env[0].ValueFrom.ConfigMapKeyRef.Key = baseCiopConfig.Filename
return ret
}(),
}},
},
},
ciop: config.CompoundCiopConfig{"org-repo-branch.yaml": &cioperatorapi.ReleaseBuildConfiguration{}},
ciop: config.CompoundCiopConfig{baseCiopConfig.Filename: &cioperatorapi.ReleaseBuildConfiguration{}},
expected: config.Presubmits{"org/repo": {
func() prowconfig.Presubmit {
ret := prowconfig.Presubmit{}
deepcopy.Copy(&ret, &basePresubmitWithCiop)
ret.Name = "job-for-org-repo"
ret.Spec.Containers[0].Env[0].ValueFrom.ConfigMapKeyRef.Key = "org-repo-branch.yaml"
ret.Spec.Containers[0].Env[0].ValueFrom.ConfigMapKeyRef.Key = baseCiopConfig.Filename
return ret
}(),
}},
Expand All @@ -459,7 +466,7 @@ func TestGetPresubmitsForCiopConfigs(t *testing.T) {
ret := prowconfig.Presubmit{}
deepcopy.Copy(&ret, &basePresubmitWithCiop)
ret.Name = "job-for-org-repo"
ret.Spec.Containers[0].Env[0].ValueFrom.ConfigMapKeyRef.Key = "org-repo-branch.yaml"
ret.Spec.Containers[0].Env[0].ValueFrom.ConfigMapKeyRef.Key = baseCiopConfig.Filename
return ret
}(),
}},
Expand Down
2 changes: 1 addition & 1 deletion pkg/rehearse/jobs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func inlineCiOpConfig(job *prowconfig.Presubmit, targetRepo string, ciopConfigs
if env.ValueFrom.ConfigMapKeyRef == nil {
continue
}
if env.ValueFrom.ConfigMapKeyRef.Name == config.CiOperatorConfigsCMName {
if config.IsCiopConfigCM(env.ValueFrom.ConfigMapKeyRef.Name) {
filename := env.ValueFrom.ConfigMapKeyRef.Key

logFields := logrus.Fields{logCiopConfigFile: filename, logCiopConfigRepo: targetRepo, logRehearsalJob: job.Name}
Expand Down
9 changes: 7 additions & 2 deletions pkg/rehearse/jobs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ func makeCMReference(cmName, key string) *v1.EnvVarSource {

func TestInlineCiopConfig(t *testing.T) {
testTargetRepo := "org/repo"
testCiopConfigInfo := config.Info{
Org: "org",
Repo: "repo",
Branch: "master",
}
testCiopConfig := &api.ReleaseBuildConfiguration{}
testCiopCongigContent, err := yaml.Marshal(testCiopConfig)
if err != nil {
Expand Down Expand Up @@ -89,12 +94,12 @@ func TestInlineCiopConfig(t *testing.T) {
expectedEnv: []v1.EnvVar{{Name: "T", ValueFrom: makeCMReference("test-cm", "key")}},
}, {
description: "CM reference to ci-operator-configs -> cm content inlined",
sourceEnv: []v1.EnvVar{{Name: "T", ValueFrom: makeCMReference(config.CiOperatorConfigsCMName, "filename")}},
sourceEnv: []v1.EnvVar{{Name: "T", ValueFrom: makeCMReference(testCiopConfigInfo.ConfigMapName(), "filename")}},
configs: config.CompoundCiopConfig{"filename": testCiopConfig},
expectedEnv: []v1.EnvVar{{Name: "T", Value: string(testCiopCongigContent)}},
}, {
description: "bad CM key is handled",
sourceEnv: []v1.EnvVar{{Name: "T", ValueFrom: makeCMReference(config.CiOperatorConfigsCMName, "filename")}},
sourceEnv: []v1.EnvVar{{Name: "T", ValueFrom: makeCMReference(testCiopConfigInfo.ConfigMapName(), "filename")}},
configs: config.CompoundCiopConfig{},
expectedError: true,
},
Expand Down
3 changes: 1 addition & 2 deletions ...ration/candidate/ci-operator/jobs/super/duper/super-duper-ciop-cfg-change-presubmits.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,14 @@ presubmits:
- --artifact-dir=$(ARTIFACTS)
- --give-pr-author-access-to-namespace=true
- --target=[images]
- --target=[release:latest]
Copy link
Contributor

@stevekuznetsov stevekuznetsov Mar 7, 2019

Choose a reason for hiding this comment

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

This is surprising -- @smarterclayton ?

Copy link
Member Author

@petr-muller petr-muller Mar 8, 2019

Choose a reason for hiding this comment

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

It's expected after regeneration - the source file is https://github.com/openshift/ci-operator-prowgen/blob/master/test/pj-rehearse-integration/candidate/ci-operator/config/super/duper/super-duper-ciop-cfg-change.yaml - it does not have a promotion stanza so prowgen does not generate this target, because such component would not promote any images.

command:
- ci-operator
env:
- name: CONFIG_SPEC
valueFrom:
configMapKeyRef:
key: super-duper-ciop-cfg-change.yaml
name: ci-operator-configs
name: ci-operator-misc-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down
15 changes: 6 additions & 9 deletions ...rse-integration/candidate/ci-operator/jobs/super/duper/super-duper-master-presubmits.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down Expand Up @@ -61,7 +61,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
- name: JOB_NAME_SAFE
value: e2e-aws
- name: TEST_COMMAND
Expand Down Expand Up @@ -107,25 +107,22 @@ presubmits:
- --artifact-dir=$(ARTIFACTS)
- --give-pr-author-access-to-namespace=true
- --target=[images]
- --target=[release:latest]
command:
- ci-operator
env:
- name: CONFIG_SPEC
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
resources:
limits:
cpu: 500m
requests:
cpu: 10m
serviceAccountName: ci-operator
trigger: ((?m)^/test( all| images),?(\s+|$))
trigger: '(?m)^/test (?:.*? )?images(?: .*?)?$'
Copy link
Contributor

@stevekuznetsov stevekuznetsov Mar 7, 2019

Choose a reason for hiding this comment

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

How was this not broken before?

Copy link
Member Author

@petr-muller petr-muller Mar 8, 2019

Choose a reason for hiding this comment

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

They are just test jobs for rehearse, I believe this field is not used anywhere.

- agent: kubernetes
always_run: true
branches:
Expand All @@ -149,7 +146,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down Expand Up @@ -183,7 +180,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down
17 changes: 7 additions & 10 deletions ...integration/candidate/ci-operator/jobs/super/trooper/super-trooper-master-presubmits.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-trooper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down Expand Up @@ -61,7 +61,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-trooper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
- name: JOB_NAME_SAFE
value: e2e-aws
- name: TEST_COMMAND
Expand Down Expand Up @@ -107,25 +107,22 @@ presubmits:
- --artifact-dir=$(ARTIFACTS)
- --give-pr-author-access-to-namespace=true
- --target=[images]
- --target=[release:latest]
command:
- ci-operator
env:
- name: CONFIG_SPEC
valueFrom:
configMapKeyRef:
key: super-trooper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
resources:
limits:
cpu: 1500m
requests:
cpu: 10m
cpu: 10000000000m
serviceAccountName: ci-operator
trigger: ((?m)^/test( all| images),?(\s+|$))
trigger: '(?m)^/test (?:.*? )?images(?: .*?)?$'
- agent: kubernetes
always_run: true
branches:
Expand All @@ -149,7 +146,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-trooper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down Expand Up @@ -183,7 +180,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-trooper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down
6 changes: 1 addition & 5 deletions test/pj-rehearse-integration/expected_jobs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
- --artifact-dir=$(ARTIFACTS)
- --give-pr-author-access-to-namespace=true
- --target=[images]
- --target=[release:latest]
- --git-ref=super/duper@ciop-cfg-change
command:
- ci-operator
Expand Down Expand Up @@ -435,7 +434,6 @@
- --artifact-dir=$(ARTIFACTS)
- --give-pr-author-access-to-namespace=true
- --target=[images]
- --target=[release:latest]
- --git-ref=super/trooper@master
command:
- ci-operator
Expand Down Expand Up @@ -471,10 +469,8 @@
imagePullPolicy: Always
name: ""
resources:
limits:
cpu: 1500m
requests:
cpu: 10m
cpu: 10M
serviceAccountName: ci-operator
refs:
base_ref: master
Expand Down
3 changes: 1 addition & 2 deletions ...tegration/master/ci-operator/jobs/super/duper/super-duper-ciop-cfg-change-presubmits.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,14 @@ presubmits:
- --artifact-dir=$(ARTIFACTS)
- --give-pr-author-access-to-namespace=true
- --target=[images]
- --target=[release:latest]
command:
- ci-operator
env:
- name: CONFIG_SPEC
valueFrom:
configMapKeyRef:
key: super-duper-ciop-cfg-change.yaml
name: ci-operator-configs
name: ci-operator-misc-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down
15 changes: 6 additions & 9 deletions ...hearse-integration/master/ci-operator/jobs/super/duper/super-duper-master-presubmits.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down Expand Up @@ -61,7 +61,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
- name: JOB_NAME_SAFE
value: e2e-aws
- name: TEST_COMMAND
Expand Down Expand Up @@ -107,25 +107,22 @@ presubmits:
- --artifact-dir=$(ARTIFACTS)
- --give-pr-author-access-to-namespace=true
- --target=[images]
- --target=[release:latest]
command:
- ci-operator
env:
- name: CONFIG_SPEC
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
resources:
limits:
cpu: 500m
requests:
cpu: 10m
serviceAccountName: ci-operator
trigger: ((?m)^/test( all| images),?(\s+|$))
trigger: '(?m)^/test (?:.*? )?images(?: .*?)?$'
- agent: kubernetes
always_run: true
branches:
Expand All @@ -149,7 +146,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down Expand Up @@ -183,7 +180,7 @@ presubmits:
valueFrom:
configMapKeyRef:
key: super-duper-master.yaml
name: ci-operator-configs
name: ci-operator-master-configs
image: ci-operator:latest
imagePullPolicy: Always
name: ""
Expand Down
Loading