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

Fixed: JENKINS-40925 - dir context is not honored by shell step #146

Merged
merged 3 commits into from Mar 10, 2017

Conversation

Projects
None yet
3 participants
@electroma

electroma commented Mar 2, 2017

It looks too easy.
Please let me know if there is a reason not to use command's pwd.

@iocanel

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel Mar 2, 2017

I am not familiar with dir context, but the code looks good and tests seem to pass.
@carlossg can you have a look too?

iocanel commented Mar 2, 2017

I am not familiar with dir context, but the code looks good and tests seem to pass.
@carlossg can you have a look too?

@electroma

This comment has been minimized.

Show comment
Hide comment
@electroma

electroma Mar 8, 2017

@carlossg can you have a look please?

electroma commented Mar 8, 2017

@carlossg can you have a look please?

@carlossg

Just a minor comment. If minikube tests pass then it's good

@@ -72,7 +70,6 @@ public ContainerExecDecorator(KubernetesClient client, String podName, String co
this.client = client;
this.podName = podName;
this.containerName = containerName;
this.path = path;

This comment has been minimized.

@carlossg

carlossg Mar 8, 2017

if path is removed, then the constructor should be deprecated and a new one without path argument added

@carlossg

carlossg Mar 8, 2017

if path is removed, then the constructor should be deprecated and a new one without path argument added

This comment has been minimized.

@electroma

electroma Mar 8, 2017

@carlossg there is only one usage of this constructor, so I can just change the signature inplace.
Do I still need to deprecate and create new one?

@electroma

electroma Mar 8, 2017

@carlossg there is only one usage of this constructor, so I can just change the signature inplace.
Do I still need to deprecate and create new one?

This comment has been minimized.

@electroma

electroma Mar 8, 2017

I've removed unused parameter

@electroma

electroma Mar 8, 2017

I've removed unused parameter

This comment has been minimized.

@carlossg

carlossg Mar 8, 2017

yes, better to copy and deprecate to keep backwards binary compatibility

@carlossg

carlossg Mar 8, 2017

yes, better to copy and deprecate to keep backwards binary compatibility

This comment has been minimized.

@electroma
@electroma

This comment has been minimized.

@electroma
@electroma

@carlossg carlossg merged commit 27280f1 into jenkinsci:master Mar 10, 2017

1 check passed

Jenkins This pull request looks good
Details

rawlingsj added a commit to fabric8io/kubernetes-plugin that referenced this pull request Apr 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment