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 multiple docker parameters and task labels #1169

Merged
merged 14 commits into from Sep 2, 2016

Conversation

tpetr
Copy link
Contributor

@tpetr tpetr commented Jul 25, 2016

Updates Docker parameters and task labels to support multiple values for a specific key. Still testing this out.

Fixes #1074

@ssalinas
Copy link
Member

@ssalinas ssalinas commented Aug 1, 2016

@tpetr it seems the @JsonDeserialize parameter wasn't playing nice with the List<T>. It was trying to deserialize each individual SingularityDockerParameter with the custom class, not the entire list. Serialization was a bit odd also.

In cef1df4 I implemented some 'wrapper' classes for handling the custom (de)serialization. Tested it and the old Map<String, String> as well as the list of parameters was working locally. Let me know what you think of this method of getting it working.

@tpetr tpetr changed the title WIP: support multiple docker parameters and task labels support multiple docker parameters and task labels Aug 3, 2016
@ssalinas ssalinas added the hs_qa label Aug 3, 2016
@ssalinas
Copy link
Member

@ssalinas ssalinas commented Aug 3, 2016

Note here, there was not an easy way to do this that wasn't a breaking change (that I could find). So we will need to make it clear in the release notes that while the api will accept Map<String, String> or List<SingularityDockerParameter>, it will always be returning a List<SigularityDockerParameter>, similar for task labels.

@ssalinas ssalinas modified the milestone: 0.10.0 Aug 4, 2016
@ssalinas ssalinas modified the milestones: 0.10.0, 0.11.0 Aug 19, 2016
@JsonProperty("dockerParameters") Optional<List<SingularityDockerParameter>> dockerParameters) {
if (dockerParameters.isPresent() && !dockerParameters.get().isEmpty() && parameters.isPresent() && !parameters.get().isEmpty()) {
throw new IllegalArgumentException("Can only specify one of 'parameters' or 'dockerParameters'");
}
Copy link
Contributor Author

@tpetr tpetr Aug 25, 2016

Choose a reason for hiding this comment

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

I'm a little hesitant of putting these kinds of exceptions in constructors -- if we somehow had a bogus one saved we'd be throwing every time it was deserialized. What do you think about moving this into SingularityValidator instead?

@ssalinas
Copy link
Member

@ssalinas ssalinas commented Aug 25, 2016

@tpetr moved those checks to the validator. Since I'm setting both objects when the deprecated one is set, validating by making sure that they are the same

@ssalinas ssalinas merged commit ff65145 into master Sep 2, 2016
0 of 2 checks passed
@ssalinas ssalinas deleted the multi-params-and-labels branch Sep 2, 2016
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.

None yet

2 participants