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

WIP: provide COMMIT_MESSAGE as an env var #796

Closed
wants to merge 4 commits into from

Conversation

jeff-knurek
Copy link

@jeff-knurek jeff-knurek commented Dec 11, 2019

JENKINS-52490 - Plugin should provide GIT_COMMIT_MESSAGE as Environment Variable

The commit message should be available as an env var.

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • I have interactively tested my changes

Types of changes

  • New feature (non-breaking change which adds functionality)

@vwin
Copy link

vwin commented Dec 20, 2019

I think so ~

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

This looks reasonable, though I think you may want to name the variable GIT_COMMIT_TITLE rather than GIT_COMMIT_MESSAGE based on the description in the git commit manual page where it says:

The text up to the first blank line in a commit message is treated as the commit title, and that title is used throughout Git. (emphasis added)

That would allow a future addition to deliver GIT_COMMIT_MESSAGE as the full message rather than only the first line of the message.

Also needs one or more tests to confirm that it is well-behaved.

}


Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't add extra white space in a change. The white space is already messy enough in the plugin without adding duplicated empty lines

Suggested change

@jeff-knurek
Copy link
Author

I've updated with the documentation bits that seem to be needed. I've also added a test, but the test doesn't actually pass. Spent some time trying to debug it, but may need to take a different approach.

Jeff Knurek added 2 commits December 30, 2019 09:41
* rename the env var to be aligned with the git description
* test isn't working yet as expected, still need more investigation into why

Signed-off-by: Jeff Knurek <[email protected]>
@MarkEWaite MarkEWaite added the enhancement Improvement or new feature label Dec 31, 2019
@otradovec
Copy link

@jeff-knurek I don't understand why are you asserting String with Env object. Maybe you wanted
assertEquals("Commit message should be an env var", title, getEnvVars(p).get("some_key","default"));
But I am a newbie, so I am missing something propably.

@jeff-knurek
Copy link
Author

@otradovec you're totally right. I had pushed some in-progress code from while I was debugging the test. I've now pushed what I expect to work (but doesn't)

@otradovec
Copy link

What about using GIT_COMMIT instead? I don't see GIT_COMMIT_TITLE here https://javadoc.jenkins.io/plugin/git/hudson/plugins/git/GitSCM.html
Or is that defined somewhere else?

@otradovec
Copy link

otradovec commented Jan 6, 2020

Btw this: https://javadoc.jenkins-ci.org/hudson/model/Run.html#getEnvVars--
getEnvVars is deprecated, Use getEnvironment(TaskListener) instead.

@jeff-knurek
Copy link
Author

GIT_COMMIT_TITLE is what I'm adding in this PR


Your tip about getEnvVars being deprecated gave me an idea, but it still didn't work.
Because right now, the getEnvVars is actually this one, based off the builder, and not the run:
https://github.com/jenkinsci/git-plugin/blob/master/src/test/java/hudson/plugins/git/AbstractGitTestCase.java#L282
so I tried using the one from the run, but it also doesn't have the new Env Var :(

@maneetgoyal
Copy link

Hi @MarkEWaite, is there anything remaining for this PR? It's a useful feature.

@jeff-knurek
Copy link
Author

I never got the unit tests to work. I think that's pretty critical to get this out. I'm not a Java/Jenkins developer though, so I think I've run out of things to try.

@@ -3064,6 +3064,24 @@ public void testCommitMessageIsPrintedToLogs() throws Exception {
assertThat(values, hasItem("Commit message: \"test commit\""));
}

@Test
public void testCommitMessageIsEnvVar() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to this I was trying to set environment variable for https://issues.jenkins.io/browse/JENKINS-38699. Does this test case run correctly in the current state?

@@ -1352,6 +1353,9 @@ public void checkout(Run<?, ?> build, Launcher launcher, FilePath workspace, Tas

// Needs to be after the checkout so that revToBuild is in the workspace
try {
String shortMessage = getCommitMessage(listener, git, revToBuild);
listener.getLogger().println("Commit message: \"" + shortMessage + "\"");
environment.put(GIT_COMMIT_TITLE, shortMessage);
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarkEWaite In this(https://github.com/jenkinsci/git-plugin/pull/1049/files#r583842074) pull request you mentioned with this enviroment variable is set only for the process.
I want to understand what is the scope of adding a variable to EnvVars is? Since it is being added in git plugin would it be available to just the checkout step or it will be available in subsequent steps like build etc. ? From the bug description I believe we want to use this env variable to check if we want to run the build step based on the commit message. So would this env variable be available in the pipeline script in the build step with this ?

@MarkEWaite
Copy link
Contributor

Closing in favor of #1074

@MarkEWaite MarkEWaite closed this Jul 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement or new feature
Projects
None yet
6 participants