-
Notifications
You must be signed in to change notification settings - Fork 2
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
Support custom labels in wave-api #18
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: munishchouhan <[email protected]>
Umm should it be a |
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
@pditommaso @marcodelapierre please review |
Looks good to me, thanks @munishchouhan , will confirm after reviewing the other related PRs. |
wave-api/src/test/groovy/io/seqera/wave/api/ContainerConfigTest.groovy
Outdated
Show resolved
Hide resolved
wave-api/src/test/groovy/io/seqera/wave/api/ContainerConfigTest.groovy
Outdated
Show resolved
Hide resolved
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.
see minor requests - thank you Munish
Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
Signed-off-by: Dr Marco Claudio De La Pierre <[email protected]>
@munishchouhan what do you think if we modelled In the end, this would be the same approach adopted for the environment variables, i.e. What do you think? See commit 618b846 |
Map is used because the list will allow repetitions |
Thanks for pointing out Munish, had overlooked that comment. This being said, I like your latest implementation, where the List is converted to Map only in the Wave server - it keeps the implementation of the functionality simple for API and clients. I ultimately don't see big drawbacks of using the Map there, so I am leaving to you the ultimate decision. @pditommaso this Libseqera PR is ready for merging. |
This PR will add labels in SubmitContainerTokenRequest class, so that it can be added in spack and conda docker file