-
Notifications
You must be signed in to change notification settings - Fork 54
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
Automount configmaps/secrets into devworkspaces #417
Automount configmaps/secrets into devworkspaces #417
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tinakurian The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple structural comments
ebd2ad0
to
7afeb52
Compare
04ef160
to
c311f8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general LGTM, just a few stuff that Che may also need.
But I'm OK with handling it out of the current PR
c311f8f
to
2b5899f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR in its current state looks good to me -- I'm 👍 to merge as-is and add additional features (mount as env or mount as volume) separately, or add to this PR if that's easier.
2b5899f
to
f734477
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm going to test it tomorrow (please make sure PR description is up to date).
It seems to be a good time that we start documenting devWorkspaces feature, but it's out of the current PR scope.
d9cb5e9
to
2fdb09c
Compare
2fdb09c
to
939a823
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again on OpenShift (yesterday) and both mount-as: env and the default work for me.
I did run into an issue on crc
but I think that's a me problem rather than a problem with this code. That issue also reproduced on the main branch, also.
Thanks @amisevsk I realized the issue i was seeing happens if i just use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM I left just a few minor comments
Tested and works as expected. I hope you don't mind that I updated testing instructiond
939a823
to
76cef50
Compare
76cef50
to
9b9601d
Compare
Not at all! Thanks for taking the time to update it @sleshchenko! |
/retest |
/test v7-images |
@tinakurian: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, sleshchenko, tinakurian 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 |
What does this PR do?
Automounts configmaps/secrets to all devworkspace if the automount label is present.
What issues does this PR fix or reference?
#384
Is it tested? How?