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

Version constant injection #576

Merged
merged 9 commits into from
Apr 1, 2022
Merged

Version constant injection #576

merged 9 commits into from
Apr 1, 2022

Conversation

lukellmann
Copy link
Member

This PR changes the version used in the user agent header from being hard-coded to being generated at build time using this gradle plugin.

The plugin will generate the internal top-level property dev.kord.common.BUILD_CONFIG_GENERATED_LIBRARY_VERSION and assign the value of Library.version to it. KordConstants.KORD_VERSION then makes this publicly accessible.

@lukellmann lukellmann requested a review from HopeBaron March 27, 2022 10:03
@lukellmann
Copy link
Member Author

@DRSchlaubi do you think common/build.gradle.kts is the right place for the plugin and its config? Or should I put this somewhere else?

@lukellmann lukellmann requested a review from DRSchlaubi March 27, 2022 10:08
@DRSchlaubi
Copy link
Member

DRSchlaubi commented Mar 27, 2022

Ideally all versions would be in the root file (just with apply false), but we already gave up on that when we used the precompiled scripts. We could add it to one of the scripts, so we can add it to multiple modules, but we only need it in common so it's fine

Copy link
Member

@DRSchlaubi DRSchlaubi left a comment

Choose a reason for hiding this comment

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

Would be nice to have a commit hash as well, as we use a lot of snapshots

@lukellmann
Copy link
Member Author

Would be nice to have a commit hash as well, as we use a lot of snapshots

What would the format look like?

@DRSchlaubi
Copy link
Member

Would be nice to have a commit hash as well, as we use a lot of snapshots

What would the format look like?

Simply make an additional constant?

And then maybe also include it with (Commit hash: ) in the user agent

@lukellmann
Copy link
Member Author

Simply make an additional constant?

Ah, I thought you wanted it to be part of the KORD_VERSION string.

@lukellmann
Copy link
Member Author

lukellmann commented Mar 27, 2022

Like this? 85270e9

I also can't come up with documentation for KordConstants.KORD_COMMIT_HASH, any suggestions?

@lukellmann lukellmann requested a review from DRSchlaubi March 27, 2022 14:01

/**
* whether the process has been invoked by JitPack
*/
val isJitPack get() = "true" == System.getenv("JITPACK")

val Project.commitHash: String
Copy link
Member

Choose a reason for hiding this comment

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

Github allows you to get the commit hash, why is this logic code introduced.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are referring to the GITHUB_SHA environment variable, right? This one is the full hash, @DRSchlaubi wanted to have the short one if available (retrieved by git rev-parse --short HEAD).

Copy link
Member

Choose a reason for hiding this comment

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

Github doesn't provide that? in the environment?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't search too long, maybe they do. Do you know more?

Copy link
Member

Choose a reason for hiding this comment

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

Github doesn't provide that? in the environment?

https://docs.github.com/en/actions/learn-github-actions/environment-variables#default-environment-variables only lists the long SHA

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should compute the short hash in the actions instead (like here).
It would probably also be good to provide KORD_COMMIT_HASH and KORD_SHORT_COMMIT_HASH.

@HopeBaron HopeBaron merged commit 61c3a09 into 0.8.x Apr 1, 2022
@lukellmann lukellmann deleted the features/version-injection branch April 1, 2022 22:36
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