Print Conjur version only on server startup #1590

orenbm · 2020-06-04T10:48:29Z

Connected to #1589

We recently started to print the Conjur version on server startup:

root@094a586bdea9:/opt/conjur-server# conjurctl server
CONJ00037I Conjur v1.7.1 starting up...
=> Booting Puma
=> Rails 5.2.4.3 application starting in production
=> Run `rails server -h` for more startup options

However, this message is written for every conjurctl command. For example:

root@094a586bdea9:/opt/conjur-server# conjurctl role retrieve-key cucumber:user:some-user
CONJ00037I Conjur v1.7.1 starting up...
error: role does not exist: cucumber:user:some-user
error: key retrieval failed

This PR changes the behaviour so that the message is printed only on server startup.

After the change, the output of conjurctl server is:

root@094a586bdea9:/opt/conjur-server# conjurctl server
Conjur v1.7.1 starting up...
=> Booting Puma
=> Rails 5.2.4.3 application starting in production
=> Run `rails server -h` for more startup options

and the output of conjurctl role is:

root@094a586bdea9:/opt/conjur-server# conjurctl role retrieve-key cucumber:user:some-user
error: role does not exist: cucumber:user:some-user
error: key retrieval failed

codeclimate · 2020-06-04T11:20:01Z

bin/conjur-cli.rb

@@ -72,6 +72,9 @@ def test_select
 end

 Process.fork do
+ conjur_version = (File.read(File.expand_path("../VERSION", File.dirname(__FILE__)))).strip


Don't use parentheses around a method call.

Can we use some helper variables to make this line less complex? Might solve this codeclimate warning too.

jtuttle · 2020-06-04T15:36:12Z

CHANGELOG.md

@@ -5,6 +5,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

 ## Unreleased
+### Fixed
+- The Conjur version is now printed on server startup, after running `conjurctl server`
+ ((cyberark/conjur#1590)[https://github.com/cyberark/conjur/pull/1590])


The parens are backwards here. The issue should have square brackets and the link should have parens.

jtuttle · 2020-06-04T15:36:49Z

bin/conjur-cli.rb

@@ -72,6 +72,9 @@ def test_select
 end

 Process.fork do
+ conjur_version = (File.read(File.expand_path("../VERSION", File.dirname(__FILE__)))).strip


Can we use some helper variables to make this line less complex? Might solve this codeclimate warning too.

izgeri

this broke a bunch of downstream projects that rely on conjurctl role retrieve-key

can we please add a test to this PR that validates that this method works (and retrieves a valid key for the specified role) so we catch changes that break it in conjur PRs instead of in downstream pipelines post-tag?

The print that we added didn't print the version only on server startup, and actually printed it for every `conjurctl` command. This commit changes this behaviour so the message is printed only on server startup.

codeclimate · 2020-06-07T11:50:13Z

cucumber/authenticators/features/support/hooks.rb

+ @env.each do |key, value|
+ ENV[key] = value
+ end
+end


Final newline missing.

codeclimate · 2020-06-07T11:50:13Z

cucumber/api/features/step_definitions/environment_steps.rb

+
+Given(/^I set environment variable "([^"]*)" to "([^"]*)"$/) do |variable_name, variable_value|
+ ENV[variable_name] = variable_value
+end


Final newline missing.

codeclimate · 2020-06-07T11:50:13Z

cucumber/policy/features/support/hooks.rb

+ @env.each do |key, value|
+ ENV[key] = value
+ end
+end


Final newline missing.

eladkug · 2020-06-07T12:26:58Z

cucumber/api/features/retrieve_api_key.feature

@@ -0,0 +1,8 @@
+Feature: Retrieving an API key with conjurctl
+
+ # We need to be in production environment to test this


Can you explain why we need to be in production?

because that's the environment our clients will be. When we are in dev env the error doesn't occur

eladkug · 2020-06-07T12:27:02Z

cucumber/api/features/retrieve_api_key.feature

+ # We need to be in production environment to test this
+ Scenario: Retrieve an API key
+ Given I set environment variable "RAILS_ENV" to "production"
+ And I set environment variable "CONJUR_LOG_LEVEL" to "info"


Why setting the conjur_log_level is relevant here?

because if we are in debug mode the output will contain some junk. we change the env so we are in the expected environment of the client using this tool.

o.k. Since its not suppose to influence the results.
In general i want to work with relevant steps only(in this case its not critical) because if from some reason we will fail on this step we cant say "retrieve an API key" is not working.

eladkug · 2020-06-07T12:27:10Z

cucumber/api/features/retrieve_api_key.feature

@@ -0,0 +1,8 @@
+Feature: Retrieving an API key with conjurctl


I would add another test to verify we have error when the API is wrong(role does not exist).

eladkug · 2020-06-07T12:53:16Z

cucumber/api/features/retrieve_api_key.feature

+
+ Scenario: Retrieve an API key of a non-existing user fails
+ When I retrieve an API key for user "cucumber:user:non-existing-user" using conjurctl
+ Then the stderr includes the error "role does not exist"


new line is missing

codeclimate · 2020-06-07T13:13:23Z

cucumber/api/features/step_definitions/conjurctl_steps.rb

@@ -0,0 +1,15 @@
+require 'open3'
+
+


Extra blank line detected.

codeclimate · 2020-06-07T13:13:24Z

cucumber/api/features/step_definitions/conjurctl_steps.rb

+
+When(/^I retrieve an API key for user "([^"]*)" using conjurctl$/) do |user_id|
+ command = "conjurctl role retrieve-key #{user_id}"
+ @conjurctl_stdout, @conjurctl_stderr, _ = Open3.capture3(command)


Do not use trailing _s in parallel assignment. Prefer @conjurctl_stdout, @conjurctl_stderr, = Open3.capture3(command).

codeclimate · 2020-06-07T13:13:24Z

cucumber/api/features/step_definitions/conjurctl_steps.rb

+end
+
+Then(/^the stderr includes the error "([^"]*)"$/) do |error|
+ expect(@conjurctl_stderr).to include("#{error}")


Prefer to_s over string interpolation.

Some test manipulate the env so we need to revert to the original one after each scenario.

codeclimate · 2020-06-07T14:22:33Z

Code Climate has analyzed commit 6dded0d 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 85.6% (0.0% change).

View more on Code Climate.

eladkug

Tests approved

Added tests as requested

orenbm requested a review from moticless June 4, 2020 10:48

orenbm self-assigned this Jun 4, 2020

orenbm added this to the PalmTree - sprint 2012 milestone Jun 4, 2020

moticless previously approved these changes Jun 4, 2020

View reviewed changes

orenbm dismissed moticless’s stale review via 7b08ed5 June 4, 2020 10:58

orenbm force-pushed the print-version-on-startyp branch 2 times, most recently from efa28f0 to d5c9601 Compare June 4, 2020 11:04

moticless previously approved these changes Jun 4, 2020

View reviewed changes

codeclimate bot reviewed Jun 4, 2020

View reviewed changes

orenbm requested a review from a team June 4, 2020 15:06

jtuttle previously requested changes Jun 4, 2020

View reviewed changes

izgeri previously requested changes Jun 4, 2020

View reviewed changes

jtuttle dismissed moticless’s stale review via ce08512 June 5, 2020 19:39

jtuttle force-pushed the print-version-on-startyp branch from d5c9601 to ce08512 Compare June 5, 2020 19:39

orenbm force-pushed the print-version-on-startyp branch from ce08512 to 0636557 Compare June 7, 2020 10:03

orenbm added 2 commits June 7, 2020 14:34

Print Conjur version only on server startup

755459e

The print that we added didn't print the version only on server startup, and actually printed it for every `conjurctl` command. This commit changes this behaviour so the message is printed only on server startup.

Add changelog entry for printing Conjur version

dbda185

orenbm force-pushed the print-version-on-startyp branch from 0636557 to 788aaf7 Compare June 7, 2020 11:34

codeclimate bot reviewed Jun 7, 2020

View reviewed changes

orenbm force-pushed the print-version-on-startyp branch from 788aaf7 to 9f2d9f1 Compare June 7, 2020 11:52

eladkug reviewed Jun 7, 2020

View reviewed changes

orenbm force-pushed the print-version-on-startyp branch from 9f2d9f1 to 3d91227 Compare June 7, 2020 12:51

eladkug reviewed Jun 7, 2020

View reviewed changes

orenbm force-pushed the print-version-on-startyp branch from 3d91227 to 2655d87 Compare June 7, 2020 12:57

codeclimate bot reviewed Jun 7, 2020

View reviewed changes

John Tuttle and others added 2 commits June 7, 2020 16:19

Add cucumber test for conjurctl retrieve-key

9a6e7be

Revert to original environment after each cucumber test

6dded0d

Some test manipulate the env so we need to revert to the original one after each scenario.

orenbm force-pushed the print-version-on-startyp branch from 2655d87 to 6dded0d Compare June 7, 2020 13:19

moticless approved these changes Jun 7, 2020

View reviewed changes

eladkug approved these changes Jun 7, 2020

View reviewed changes

orenbm merged commit 90b7e97 into master Jun 7, 2020

orenbm deleted the print-version-on-startyp branch June 7, 2020 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Print Conjur version only on server startup #1590

Print Conjur version only on server startup #1590

orenbm commented Jun 4, 2020 •

edited

Loading

codeclimate bot Jun 4, 2020

jtuttle Jun 4, 2020

jtuttle Jun 4, 2020

jtuttle Jun 4, 2020

izgeri left a comment

codeclimate bot Jun 7, 2020

codeclimate bot Jun 7, 2020

codeclimate bot Jun 7, 2020

eladkug Jun 7, 2020

orenbm Jun 7, 2020

eladkug Jun 7, 2020

orenbm Jun 7, 2020

eladkug Jun 7, 2020

eladkug Jun 7, 2020

orenbm Jun 7, 2020

eladkug Jun 7, 2020

codeclimate bot Jun 7, 2020

codeclimate bot Jun 7, 2020

codeclimate bot Jun 7, 2020

codeclimate bot commented Jun 7, 2020

eladkug left a comment

		@@ -0,0 +1,8 @@
		Feature: Retrieving an API key with conjurctl

		# We need to be in production environment to test this

Print Conjur version only on server startup #1590

Print Conjur version only on server startup #1590

Conversation

orenbm commented Jun 4, 2020 • edited Loading

codeclimate bot Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izgeri left a comment

Choose a reason for hiding this comment

codeclimate bot Jun 7, 2020

Choose a reason for hiding this comment

codeclimate bot Jun 7, 2020

Choose a reason for hiding this comment

codeclimate bot Jun 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Jun 7, 2020

Choose a reason for hiding this comment

codeclimate bot Jun 7, 2020

Choose a reason for hiding this comment

codeclimate bot Jun 7, 2020

Choose a reason for hiding this comment

codeclimate bot commented Jun 7, 2020

eladkug left a comment

Choose a reason for hiding this comment

orenbm commented Jun 4, 2020 •

edited

Loading