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.

Sign up for GitHub

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

Jump to bottom

JENKINS-56133: Add build url as default annotation #433

Merged
merged 1 commit into from
Feb 26, 2019
Merged

JENKINS-56133: Add build url as default annotation #433

merged 1 commit into from
Feb 26, 2019

Conversation

MattLud
Copy link
Contributor

@MattLud MattLud commented Feb 18, 2019

No description provided.

@MattLud MattLud marked this pull request as ready for review February 18, 2019 22:14
carlossg
carlossg reviewed Feb 19, 2019
View reviewed changes
src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesTestUtil.java Outdated
@@ -142,9 +142,39 @@ public static void assumeKubernetes() throws Exception {
return l;
}

public static Map<String, String> getAnnotations(KubernetesCloud cloud, String podName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you are not using this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's being used in KubernetesPipelineTest and KubernetsDeclarativeAgent test -
image

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, you are using it in the tests but not in the actual plugin, there's nothing that will add the annotations to the pods when they are launched

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The annotation is being added automatically by the step execution - https://github.com/jenkinsci/kubernetes-plugin/pull/433/files#diff-5501a99a43da60cbad908396dfee9336R100 - the method is necessary to confirm that the the annotation was added and properly reflects the build url.

Copy link
Contributor

Choose a reason for hiding this comment

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

my bad, missread it

carlossg
carlossg reviewed Feb 19, 2019
View reviewed changes
Copy link
Contributor

@carlossg carlossg left a comment

Choose a reason for hiding this comment

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

Both tests are failing in assertNotNull(annotations);

src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesTestUtil.java Outdated
@@ -142,9 +142,39 @@ public static void assumeKubernetes() throws Exception {
return l;
}

public static Map<String, String> getAnnotations(KubernetesCloud cloud, String podName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

my bad, missread it

@MattLud
Copy link
Contributor Author

MattLud commented Feb 19, 2019

Is there a link to the failed tests? Worked fine locally, but I'm worried it was an edge case.

@MattLud
Copy link
Contributor Author

MattLud commented Feb 19, 2019

Going to rewrite the tests to match runInPod approach to verifying to avoid race condition.

@carlossg carlossg merged commit 1f74da8 into jenkinsci:master Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlossg carlossg carlossg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MattLud @carlossg