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

add ENV variable that reads from VERSION file #1450

Merged
merged 2 commits into from
Mar 25, 2020

Conversation

h-artzi
Copy link
Contributor

@h-artzi h-artzi commented Mar 23, 2020

What does this PR do?

It displays the correct version on the status page. This version number is retrieved from the VERSION file

How to Test

  1. ./build.sh
  2. cd dev
  3. ./start
  4. run conjurctl server within the container
  5. go to http://localhost:3000

Screen Shot

Screen Shot 2020-03-23 at 5 13 08 PM

What ticket does this PR close?

Connected to #1438

Checklists

Test coverage

  • This PR includes new unit and integration tests to go with the code changes, or
  • The changes in this PR do not require tests

Follow-on issues

  • Follow-up issue(s) have been logged (and links included below) to update documentation or related projects, or
  • No follow-up issues are required

@h-artzi h-artzi force-pushed the update-version-on-status-page branch from 94f95f1 to 4de379d Compare March 24, 2020 19:13
Copy link
Contributor

@micahlee micahlee left a comment

Choose a reason for hiding this comment

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

Looks good! I just have a couple of small nits. Generally, I think these commits can be combined into just 1 commit, rather than 3. Is there a reason for keeping them separate?

Thanks!

@h-artzi h-artzi force-pushed the update-version-on-status-page branch 3 times, most recently from 405a938 to 777dcfe Compare March 25, 2020 16:09
The naming was chosen due to the fact that `CONJUR_VERSION` interferes
with current tests
@h-artzi h-artzi force-pushed the update-version-on-status-page branch from 777dcfe to 16ba9af Compare March 25, 2020 16:17
@h-artzi
Copy link
Contributor Author

h-artzi commented Mar 25, 2020

I just kept them separate for readability but have no problem combining them

@h-artzi h-artzi changed the title added ENV variable that reads from VERSION file add ENV variable that reads from VERSION file Mar 25, 2020
micahlee
micahlee previously approved these changes Mar 25, 2020
Copy link
Contributor

@micahlee micahlee left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@codeclimate
Copy link

codeclimate bot commented Mar 25, 2020

Code Climate has analyzed commit 708c5d9 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 89.4% (0.0% change).

View more on Code Climate.

@micahlee micahlee merged commit 666076d into master Mar 25, 2020
@micahlee micahlee deleted the update-version-on-status-page branch March 25, 2020 20:19
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.

2 participants