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.

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

munishchouhan
Copy link
Member

This PR will add labels in SubmitContainerTokenRequest class, so that it can be added in spack and conda docker file

@pditommaso
Copy link
Contributor

Umm should it be a List of pairs or Map of key-value ? a list would allow repetitions. Is this possible with docker labels?

@munishchouhan
Copy link
Member Author

If there is a repeated label, but with a different value then the latest value will be applied, so i think we can use Map here
Screenshot 2024-03-12 at 17 16 16

Signed-off-by: munishchouhan <[email protected]>
Signed-off-by: munishchouhan <[email protected]>
@munishchouhan munishchouhan self-assigned this Mar 18, 2024
@munishchouhan
Copy link
Member Author

@pditommaso @marcodelapierre please review

@marcodelapierre
Copy link
Contributor

Looks good to me, thanks @munishchouhan , will confirm after reviewing the other related PRs.

Copy link
Contributor

@marcodelapierre marcodelapierre left a 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

@marcodelapierre marcodelapierre changed the title Added labels in SubmitContainerTokenRequest Support custom labels in wave-api Apr 10, 2024
munishchouhan and others added 4 commits April 10, 2024 12:49
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]>
@marcodelapierre
Copy link
Contributor

@munishchouhan what do you think if we modelled labels as List instead of Map?

In the end, this would be the same approach adopted for the environment variables, i.e. env attribute; the two attributes look very similar to me. And then implementation of labels in Wave could closely mimick that of env.

What do you think?

See commit 618b846

@munishchouhan
Copy link
Member Author

@munishchouhan what do you think if we modelled labels as List instead of Map?

In the end, this would be the same approach adopted for the environment variables, i.e. env attribute; the two attributes look very similar to me. And then implementation of labels in Wave could closely mimick that of env.

What do you think?

See commit 618b846

Map is used because the list will allow repetitions
check this comment #18 (comment)

@marcodelapierre
Copy link
Contributor

Thanks for pointing out Munish, had overlooked that comment.
I see that Docker does work with multiple definitions of a same LABEL, by retaining the most recent one, a bit like environment variables definition in a Linux shell. As such, a List would have worked as well (same as it works for env variables).

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants