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

Don't use --short when retrieving revisions from git #1638

Merged

Conversation

dliappis
Copy link
Contributor

@dliappis dliappis commented Dec 5, 2022

Rally uses the git revision in several places, one of which when building Elasticsearch from source. Historically it used --short1 which results in shorter filenames, but they are unpredictable and more importantly cause an unnecessary git clone when using the build and install subcommands separately as described in #1635.

In this commit we switch to the full SHA length for all cases.

Closes #1635

Footnotes

  1. https://git-scm.com/docs/git-rev-parse#Documentation/git-rev-parse
    .txt---shortlength

Rally uses the git revision in several places, one of which when
building Elasticsearch from source. Historically it used `--short`[^1]
which results in shorter filenames, but they are unpredictable and more
importantly cause an unnecessary git clone when using the `build` and
`install` subcommands separately as described in elastic#1635.

In this commit we switch to the full SHA length for all cases.

Closes elastic#1635

[^1]: https://git-scm.com/docs/git-rev-parse#Documentation/git-rev-parse
      .txt---shortlength
@dliappis dliappis added bug Something's wrong :Usability Makes Rally easier to use labels Dec 5, 2022
@dliappis dliappis self-assigned this Dec 5, 2022
@dliappis
Copy link
Contributor Author

dliappis commented Dec 5, 2022

Reviewers: I spent some time thinking about what test could be meaningful for catching the referenced issue, but I can't think of a unit test that would be suitable; after all, the issue is that without this change the built artifact includes a revision that is unpredictable and possibly different from the revision passed with --revision. An IT test using a defined revision SHA IMHO would be problematic as the way that ES gets built changes over time plus it'd add unnecessary time spent building with gradle, unless we'd mock it out.

I also considered rather than removing --short SHA, to make the check more lenient; this would mean using git and therefore either cloning the repo again or using GitHub network lookups to determine whether the value passed with --revision and whatever revision value is part of the filename are the same (any of which could be a shortened version).
Cons: apart from flaky network connections, it would require knowledge of the repo URL itself which we don't store atm in the rally-teams vanicall/config.ini (the repo URL, if available, would be used for a new method to calculate the shortened revision -- e.g. see here)

Finally, this PR means that we should encourage users to pass the entire SHA when using --revision. Maybe we could strive to convert the shortened SHA to the full length SHA in a future PR?

Copy link
Contributor

@michaelbaamonde michaelbaamonde left a comment

Choose a reason for hiding this comment

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

I think this is fine as-is. I appreciate the thought you put into possible test scenarios, but I agree that they're impractical.

Re: converting a short SHA to a full one in the future, it doesn't really seem worth it to me.

@dliappis
Copy link
Contributor Author

dliappis commented Dec 7, 2022

Re: converting a short SHA to a full one in the future, it doesn't really seem worth it to me.

The motivation behind this was purely for user friendliness since some tools and UIs might emit a shortened SHA, which if passed to Rally would cause again the same unnecessary cloning described in the issue #1635 description.

@dliappis dliappis merged commit 890b761 into elastic:master Dec 7, 2022
@pquentin pquentin added this to the 2.7.1 milestone Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something's wrong :Usability Makes Rally easier to use
Projects
None yet
3 participants