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.

Sign up for GitHub

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

Jump to bottom

Add annotation for setting access mode on automounted configmap/secret files #6750

Merged
merged 4 commits into from
Apr 22, 2023
Merged

Add annotation for setting access mode on automounted configmap/secret files #6750

merged 4 commits into from
Apr 22, 2023

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Apr 18, 2023 •
edited by valaparthvi
Loading

What type of PR is this:

/kind feature

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #6549
Fixes #6728

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@netlify
Copy link

netlify bot commented Apr 18, 2023 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit ece4079
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/64425e088236010008bd0179
😎 Deploy Preview https://deploy-preview-6750--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Apr 18, 2023
@openshift-ci openshift-ci bot requested review from rm3l and valaparthvi April 18, 2023 16:08
@odo-robot
Copy link

odo-robot bot commented Apr 18, 2023 •
edited
Loading

NoCluster Tests on commit 5f99810 finished successfully.
View logs: TXT HTML

@feloy feloy changed the title Add annotation for setting access mode on automounted configmap/secret files [WIP] Add annotation for setting access mode on automounted configmap/secret files Apr 18, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Apr 18, 2023
@odo-robot
Copy link

odo-robot bot commented Apr 18, 2023 •
edited
Loading

OpenShift Unauthenticated Tests on commit 5f99810 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 18, 2023 •
edited
Loading

Unit Tests on commit 5f99810 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 18, 2023 •
edited
Loading

Validate Tests on commit 5f99810 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 18, 2023 •
edited
Loading

Windows Tests (OCP) on commit 5f99810 finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 18, 2023 •
edited
Loading

OpenShift Tests on commit 5f99810 finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 19, 2023 •
edited
Loading

Kubernetes Tests on commit 5f99810 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 19, 2023 •
edited
Loading

Kubernetes Docs Tests on commit bd5098e finished successfully.
View logs: TXT HTML

@feloy feloy changed the title [WIP] Add annotation for setting access mode on automounted configmap/secret files Add annotation for setting access mode on automounted configmap/secret files Apr 19, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Apr 19, 2023
rm3l
rm3l requested changes Apr 20, 2023
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

Can you also add this new annotation to the related documentation page?
https://deploy-preview-6750--odo-docusaurus-preview.netlify.app/docs/user-guides/advanced/automounting-volumes/

@feloy feloy requested a review from rm3l April 20, 2023 17:13
rm3l
rm3l reviewed Apr 21, 2023
View reviewed changes
docs/website/docs/user-guides/advanced/automounting-volumes.md Outdated
@@ -37,3 +37,4 @@ Mounting resources can be additionally configured via annotations:

- `devfile.io/read-only`: for persistent volume claims, mount the resource as read-only

- `devfile.io/mount-access-mode`: for secret/configmap, can be used to configure file permissions on mounted files
Copy link
Member

@rm3l rm3l Apr 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you also provide an example value? Along with the valid range?

This makes me wonder that we could add a complete example of an auto-mounting resource, with its labels and annotations. That would be helpful and easier to understand, IMO. But we can do that in a separate PR if you prefer..

LGTM otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've completed the doc, and added a test for decimal notation.

I would prefer we discuss which kind of example we want and where to place it (blog, etc), and to create a separate PR for this.

@feloy feloy requested a review from rm3l April 21, 2023 09:59
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 21, 2023
@feloy feloy closed this Apr 21, 2023
@feloy feloy reopened this Apr 21, 2023
@feloy feloy closed this Apr 21, 2023
@feloy feloy reopened this Apr 21, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@feloy
Copy link
Contributor Author

feloy commented Apr 22, 2023

/override windows-integration-test/Windows-test

FAILED] [24.026 seconds]
odo devfile deploy command tests [AfterEach] when using a devfile.yaml containing an Image component with no build context should build image via Podman by defaulting build context to devfile path
  [AfterEach]

@openshift-ci
Copy link

openshift-ci bot commented Apr 22, 2023

@feloy: Overrode contexts on behalf of feloy: windows-integration-test/Windows-test

In response to this:

/override windows-integration-test/Windows-test

FAILED] [24.026 seconds]
odo devfile deploy command tests [AfterEach] when using a devfile.yaml containing an Image component with no build context should build image via Podman by defaulting build context to devfile path
 [AfterEach]

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/test-infra repository.

@feloy
Copy link
Contributor Author

feloy commented Apr 22, 2023

/override OpenShift-Integration-tests/OpenShift-Integration-tests

Unrelated tests

[FAILED] [373.200 seconds]
odo describe/list binding command tests when creating a component with a binding as environment variables (service in namespace "") when Starting a Pg service when running dev session [BeforeEach] when changing the current namespace should list the binding with --namespace flag
  [BeforeEach] /go/odo_1/tests/integration/cmd_describe_list_binding_test.go:690
  [It] /go/odo_1/tests/integration/cmd_describe_list_binding_test.go:787

FAILED] [372.686 seconds]
odo describe/list binding command tests when creating a component with a binding as environment variables (service in namespace "binding-ncr") when Starting a Pg service when running dev session [BeforeEach] should list the binding
  [BeforeEach] /go/odo_1/tests/integration/cmd_describe_list_binding_test.go:690
  [It] /go/odo_1/tests/integration/cmd_describe_list_binding_test.go:742

@openshift-ci
Copy link

openshift-ci bot commented Apr 22, 2023

@feloy: Overrode contexts on behalf of feloy: OpenShift-Integration-tests/OpenShift-Integration-tests

In response to this:

/override OpenShift-Integration-tests/OpenShift-Integration-tests

Unrelated tests

[FAILED] [373.200 seconds]
odo describe/list binding command tests when creating a component with a binding as environment variables (service in namespace "") when Starting a Pg service when running dev session [BeforeEach] when changing the current namespace should list the binding with --namespace flag
 [BeforeEach] /go/odo_1/tests/integration/cmd_describe_list_binding_test.go:690
 [It] /go/odo_1/tests/integration/cmd_describe_list_binding_test.go:787

FAILED] [372.686 seconds]
odo describe/list binding command tests when creating a component with a binding as environment variables (service in namespace "binding-ncr") when Starting a Pg service when running dev session [BeforeEach] should list the binding
 [BeforeEach] /go/odo_1/tests/integration/cmd_describe_list_binding_test.go:690
 [It] /go/odo_1/tests/integration/cmd_describe_list_binding_test.go:742

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/test-infra repository.

@openshift-ci openshift-ci bot merged commit 61c38c4 into redhat-developer:main Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rm3l rm3l rm3l approved these changes

@valaparthvi valaparthvi Awaiting requested review from valaparthvi

Assignees

@rm3l rm3l

Labels
kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
2 participants
@feloy @rm3l