Skip to content

BUILD-87: Add Build Volumes API with validation#208

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
coreydaley:jira_build_87_secret_configmap_volume_mounts_in_builds
Jun 27, 2021
Merged

BUILD-87: Add Build Volumes API with validation#208
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
coreydaley:jira_build_87_secret_configmap_volume_mounts_in_builds

Conversation

@coreydaley
Copy link

@coreydaley coreydaley commented May 11, 2021

@coreydaley
Copy link
Author

coreydaley commented May 11, 2021

/assign @adambkaplan @gabemontero

@openshift-ci openshift-ci bot requested review from deads2k and sttts May 11, 2021 16:55
@coreydaley
Copy link
Author

coreydaley commented May 13, 2021

/retest

adambkaplan
adambkaplan suggested changes May 13, 2021
View reviewed changes
@coreydaley
Copy link
Author

coreydaley commented May 13, 2021

/retest

adambkaplan
adambkaplan reviewed May 14, 2021
View reviewed changes
@gabemontero
Copy link
Contributor

gabemontero commented May 14, 2021

Did you run make update @coreydaley and store those changes in a separate commit? It was hard for me to tell with what you currently have here.

If you have not, you'll need that to get your ci/prow/verify runs clean.

@adambkaplan
Copy link
Contributor

adambkaplan commented May 18, 2021

/hold

Given the complexity of the feature and the time remaining before 4.8 reaches "final freeze" period, we are going to hold this until master opens for 4.9

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2021
@coreydaley
Copy link
Author

coreydaley commented May 21, 2021

/retest

@coreydaley coreydaley changed the title bump(*) BUILD-87: bump openshift/api May 21, 2021
@coreydaley
Copy link
Author

coreydaley commented May 24, 2021

/retest

adambkaplan
adambkaplan suggested changes May 24, 2021
View reviewed changes
@coreydaley
Copy link
Author

coreydaley commented May 26, 2021

/retest

4 similar comments
@coreydaley
Copy link
Author

coreydaley commented May 27, 2021

/retest

@coreydaley
Copy link
Author

coreydaley commented May 27, 2021

/retest

@coreydaley
Copy link
Author

coreydaley commented May 28, 2021

/retest

@coreydaley
Copy link
Author

coreydaley commented Jun 1, 2021

/retest

@adambkaplan
Copy link
Contributor

adambkaplan commented Jun 1, 2021

@coreydaley looks like the upgrade tests are failing for unrelated reasons. I think it is time to start getting QE to test this new feature, and to write the CI tests that we will add to origin.

@xiuwang for testing this feature, we will want to modify the existing build tests for Secret and ConfigMap build inputs to use the new volumes API. BuildConfig specs will look like this:

spec:
  source:
    git: ...
  strategy:
    sourceStrategy:
      ...
      volumes:
        - name: some-secret
          source:
            secret:
              secretName: my-secret
          mounts:
             - destinationPath: /var/run/secret/some-secret
               readOnly: true

PRs needed to test this feature:

adambkaplan
adambkaplan reviewed Jun 1, 2021
View reviewed changes
Copy link
Contributor

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/approve

Marking approve to indicate we are ready for QE to begin their verification.

@coreydaley
Copy link
Author

coreydaley commented Jun 2, 2021

/retest

@adambkaplan
Copy link
Contributor

adambkaplan commented Jun 2, 2021 •
edited
Loading

Given the first test by @xiuwang, we need to add validation on the volume name. It needs to have a valid DNS label.

Validations should be added here (using a common method):

  1. https://github.com/openshift/openshift-apiserver/blob/master/pkg/build/apis/build/validation/validation.go#L563-L569
  2. https://github.com/openshift/openshift-apiserver/blob/master/pkg/build/apis/build/validation/validation.go#L519-L547

Upstream k8s/apimachinery has a method which lets us validate the name is an appropriate DNS label (required by kube for volumes): https://github.com/kubernetes/apimachinery/blob/master/pkg/util/validation/validation.go#L187

@coreydaley
Copy link
Author

coreydaley commented Jun 2, 2021

@adambkaplan Added validation and tests in current push. ptal

@coreydaley
Copy link
Author

coreydaley commented Jun 2, 2021

/retest

2 similar comments
@coreydaley
Copy link
Author

coreydaley commented Jun 2, 2021

/retest

@coreydaley
Copy link
Author

coreydaley commented Jun 2, 2021

/retest

@xiuwang
Copy link

xiuwang commented Jun 11, 2021

I have test the feature following test cases https://url.corp.redhat.com/buildvolumes, no critial issue found.

/label qe-approved

@coreydaley
Copy link
Author

coreydaley commented Jun 25, 2021

@sttts I think we are ready for an approval, the e2e-aws-upgrade test failures are unrelated and being discussed on slack.

sttts
sttts reviewed Jun 25, 2021
View reviewed changes
sttts
sttts reviewed Jun 25, 2021
View reviewed changes
sttts
sttts reviewed Jun 25, 2021
View reviewed changes
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2021
sttts
sttts reviewed Jun 25, 2021
View reviewed changes
@sttts
Copy link
Contributor

sttts commented Jun 25, 2021

A couple of validation formalities are wrong.

@coreydaley
Copy link
Author

coreydaley commented Jun 25, 2021

@sttts All comments have been addressed, thanks for the review

sttts
sttts reviewed Jun 25, 2021
View reviewed changes
@coreydaley
- updates corresponding in-tree api definition
f4333d4 
- adds validations for new api fields
- adds tests for new validations
@coreydaley
Copy link
Author

coreydaley commented Jun 26, 2021

/retest

sttts
sttts reviewed Jun 26, 2021
View reviewed changes
pkg/build/apis/build/validation/validation.go
allErrs = append(allErrs, field.Invalid(fldPath.Child("source"), volume.Source, "only one volume source is allowed"))
}
if len(foundVolumeSources) != 0 && foundVolumeSources[0] != string(volume.Source.Type) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("source").Child("type"), volume.Source.Type, "source type and specified type must match"))
Copy link
Contributor

@sttts sttts Jun 26, 2021

Choose a reason for hiding this comment

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

this and the one above is still not of shape must ..... Follow-up is fine.

@sttts
Copy link
Contributor

sttts commented Jun 26, 2021

/retest
/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, coreydaley, gabemontero, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2021
@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 26, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

12 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 26, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 26, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 26, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 26, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 26, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 27, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 27, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 27, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 27, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 27, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 27, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 27, 2021

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 2b2689b into openshift:master Jun 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deads2k deads2k Awaiting requested review from deads2k

3 more reviewers

@sttts sttts sttts left review comments

@gabemontero gabemontero gabemontero left review comments

@adambkaplan adambkaplan adambkaplan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sttts sttts

@adambkaplan adambkaplan

@gabemontero gabemontero

@deads2k deads2k

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@coreydaley @gabemontero @adambkaplan @xiuwang @RickJWagner @sttts @openshift-bot @deads2k @openshift-merge-robot

Comments