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

Set version with git #735

Merged
merged 3 commits into from
Aug 2, 2022
Merged

Set version with git #735

merged 3 commits into from
Aug 2, 2022

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Aug 1, 2022

In order to get the version of the rolling container, this PR includes a new way of getting the application version. It uses the next order:

  1. VERSION env variable during the release creation
  2. hack/get_version_from_git.sh script output
  3. Default version

image

PD: I have already checked solutions like edeliver, but they look more complicated than the thing we really need. Anyway, if there is any better way of doing so, I'm all open to apply it!

@arbulu89 arbulu89 added the enhancement New feature or request label Aug 1, 2022
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. Just a tiny thought to better understand.
I am also open to learn for better ways of doing it 😄

mix.exs Outdated
end

defp get_version_from_git do
case File.cwd!() |> Path.join("hack/get_version_from_git.sh") |> System.cmd([]) do
Copy link
Member

@nelsonkopliku nelsonkopliku Aug 1, 2022

Choose a reason for hiding this comment

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

I am wondering whether this is necessary here.
What I mean is that we're setting the VERSION env variable at build time by using get_version_from_git.sh so, do we really need to do that here? 🤔

The alternative I see would be: either at build time the VERSION has been set, or we fallback to the default version.

Am I missing something? would that work? Would that even make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I guess you are right. I just implemented this way to have the way to debug it in my development environment.
We can delete this case, no issues with it.

Ok, I will remove it for the sake of simplicity

Copy link
Contributor Author

@arbulu89 arbulu89 Aug 1, 2022

Choose a reason for hiding this comment

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

Edit: Ok, now I know why I have added it...
It was for the community version. The VERSION replacement only affect the suse container, but the community doesn't have such a replacement or Dockerfile edition, and the docker/build-push-action@v3 job doesn't make it that straightforward.

@nelsonkopliku This makes me reconsider the removal of the script usage in the code hehe, as I don't like having string replacements in the community dockerfile. I will check what I can do there, maybe I can use env variables or something like that...

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, is that the reason why e2e tests are failing?

Anyway, no strong opinion from my side. It is just fine like you did. 💪

Copy link
Member

@fabriziosestito fabriziosestito Aug 1, 2022

Choose a reason for hiding this comment

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

and the docker/build-push-action@v3 job doesn't make it that straightforward.

ah I see, have to say I am not a big fan of executing shell scripts from elixir but if there is not another way i am ok with it

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

LGTM

@arbulu89 arbulu89 force-pushed the set-version-with-git branch from 0ae42db to 13319a8 Compare August 1, 2022 14:40
mix.exs Outdated Show resolved Hide resolved
mix.exs Outdated Show resolved Hide resolved
@arbulu89 arbulu89 force-pushed the set-version-with-git branch from ce04477 to 6500dcd Compare August 2, 2022 06:39
@arbulu89 arbulu89 merged commit e9881db into main Aug 2, 2022
@arbulu89 arbulu89 deleted the set-version-with-git branch August 2, 2022 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants