Skip to content

Conversation

@Hendrik-H
Copy link
Contributor

@Hendrik-H Hendrik-H commented Sep 16, 2016

The code change is based on the code for issue #688, which is about the same code area.


This change is Reviewable

@Hendrik-H Hendrik-H changed the title Issue 554 Support volumes propagation flags (Issue 554) Sep 17, 2016
*/
public enum PropagationMode {
/** default */
mount_default(""),
Copy link
Member

Choose a reason for hiding this comment

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

Name as java convention and annotate with JsonProperty according to real field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is that the values are default and private, which is not allowed. So how should they be called then?
Do you have an example for the JsonProperty stuff? I tried to make this analogue to SELContext.java.

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 renamed the values but could not figure out where the JsonProperty annotation should go here. My code should be in line with AccessMode and SELContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please let me know if anything is missing. If something about Json is required then please let me know what exactly, ideally with a pointer to other similar code. Would be great if a version with this change could be released soon.

@KostyaSha
Copy link
Member

LGTM but will back on weekend, sorry.

@KostyaSha
Copy link
Member

#696 is the same options right? Could you review each other PR?

@Hendrik-H
Copy link
Contributor Author

I had based my change on the branch for the other fix. So my branch contains the the same commit.
The other change looked good to me.

@dmandalidis
Copy link

@Hendrik-H please cherry-pick again that initial commit from #696 as it contains an integration test which is missing in your case.

@KostyaSha this PR would be more complete in terms of features. However, I believe you could have pointed out to both of us that we 're doing the same thing in parallel while you were reviewing both PRs at the same time

@Hendrik-H
Copy link
Contributor Author

Hi, I can only look into it on Monday. From what I had seen so far we added different things in the same area. Can't currently check the code. I thought one could first pull in the other change and then mine as I had based mine in the earlier commits.

On September 30, 2016 7:36:23 AM GMT+02:00, Dimitris Mandalidis [email protected] wrote:

@Hendrik-H please cherry-pick again that initial commit from #696 as
it contains an integration test which is missing in your case.

@KostyaSha this PR would be more complete in terms of features.
However, I believe you could have pointed out to both of us that we 're
doing the same thing in parallel while you were reviewing both PRs at
the same time

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#705 (comment)

@Hendrik-H
Copy link
Contributor Author

I rebased my change now on top of the latest version of https://github.com/dmandalidis/docker-java/tree/issue-688. I'm still a bit new to git, so hope to have done it right ...

@dmandalidis
Copy link

Seems ok, please fixup your latest commit with the one that introduced the mistake.

@Hendrik-H
Copy link
Contributor Author

sorry, no idea how I would do that. looks like I had the wrong version in 5abe479. so would somehow have to redo the whole thing. what's the problem of an additional commit ?

@dmandalidis
Copy link

5abe479 introduced the wrong version. PR would be more clear that way, but again it's just my opinion :) See manual of git-rebase (along with the interactive option)

@Hendrik-H
Copy link
Contributor Author

ok, I did a git rebase --interactive 5abe479, changed the order of the commits and changed the version fix from a pick to a fixup.

@Hendrik-H
Copy link
Contributor Author

@KostyaSha anything missing to get this merged?

@KostyaSha KostyaSha merged commit 5736b4b into docker-java:master Oct 7, 2016
@KostyaSha KostyaSha added this to the 3.0.7 milestone Oct 7, 2016
Yogu pushed a commit to Yogu/docker-java that referenced this pull request Nov 15, 2016
* Support nocopy option in volume binds (docker-java#688)

* support volumes propagation flags, fixes docker-java#554

* renamed values to match java convention
panuse pushed a commit to TuKangTech/docker-java that referenced this pull request Aug 20, 2017
* Support nocopy option in volume binds (docker-java#688)

* support volumes propagation flags, fixes docker-java#554

* renamed values to match java convention
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