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

Remove the stale puma pidfile before starting the server process #2498

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

ajay-vel
Copy link
Contributor

@ajay-vel ajay-vel commented Feb 16, 2022

Desired Outcome

Implemented Changes

  • A new method is added to conjur-cli to check if the stale puma pid file exists and deletes it if it exists.

Connected Issue/Story

Definition of Done

  • A new method checks if the stale pidfile exists and it is removed before the starting the conjur server.

Changelog

  • The CHANGELOG has been updated with the bug that was fixed.

Test coverage

  • This PR includes two new unit tests to go with the code
    changes

Documentation

  • This PR does not require updating any documentation

Behavior

  • No behavior was changed with this PR

Security

  • There are no security aspects to these changes

@ajay-vel ajay-vel requested a review from a team as a code owner February 16, 2022 16:20
bin/conjur-cli/commands/server.rb Outdated Show resolved Hide resolved
bin/conjur-cli/commands/server.rb Outdated Show resolved Hide resolved
bin/conjur-cli/commands/server.rb Outdated Show resolved Hide resolved
@micahlee
Copy link
Contributor

Hey @ajay-vel , another thing I just realized is that this PR description is not the standard template. Can you please use that instead?

There are some important questions there for both you and the reviewer to consider for each PR. For example, this change does need entry in the CHANGELOG.

Thanks!

### Desired Outcome

*Please describe the desired outcome for this PR.  Said another way, what was
the original request that resulted in these code changes?  Feel free to copy
this information from the connected issue.*

### Implemented Changes

*Describe how the desired outcome above has been achieved with this PR. In
particular, consider:*

- _What's changed? Why were these changes made?_
- _How should the reviewer approach this PR, especially if manual tests are required?_
- _Are there relevant screenshots you can add to the PR description?_

### Connected Issue/Story

Resolves #[relevant GitHub issue(s), e.g. 76]

CyberArk internal issue link: [insert issue ID]()

### Definition of Done
*At least 1 todo must be completed in the sections below for the PR to be
merged.*

#### Changelog

- [ ] The CHANGELOG has been updated, or
- [ ] This PR does not include user-facing changes and doesn't require a
  CHANGELOG update

#### 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

#### Documentation

- [ ] Docs (e.g. `README`s) were updated in this PR
- [ ] A follow-up issue to update official docs has been filed here: [insert issue ID]()
- [ ] This PR does not require updating any documentation

#### Behavior

- [ ] This PR changes product behavior and has been reviewed by a PO, or
- [ ] These changes are part of a larger initiative that will be reviewed later, or
- [ ] No behavior was changed with this PR

#### Security

- [ ] Security architect has reviewed the changes in this PR,
- [ ] These changes are part of a larger initiative with a separate security review, or
- [ ] There are no security aspects to these changes 

@micahlee
Copy link
Contributor

One other thing I remembered seeing the PR checklist again is that we should add tests for this change. They can probably go in the existing spec suite here: https://github.com/cyberark/conjur/blob/master/spec/conjurctl/server_spec.rb

We should verify the behavior:

  • when the PID file does exist
  • when the PID file does not exist

@ajay-vel ajay-vel changed the title Remove the Pidfile before starting the server process Remove the stale puma pidfile before starting the server process Mar 1, 2022
@ajay-vel ajay-vel force-pushed the cyberark/ONYX-16928 branch 4 times, most recently from c1e405a to e0b420b Compare March 1, 2022 23:15
CHANGELOG.md Outdated Show resolved Hide resolved
@ajay-vel ajay-vel force-pushed the cyberark/ONYX-16928 branch 3 times, most recently from 6e05df4 to 5331844 Compare March 2, 2022 23:26
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.

Hey @ajay-vel , thanks for updating the tests! One last comment on the changelog, and this should be good to go!

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -47,6 +47,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

### Fixed
- IAM Authn bug fix - Take rexml gem to production configuration [#2493](https://github.com/cyberark/conjur/pull/2493)
- Previously, a stale puma pid file would prevent the Conjur server from starting
Copy link

Choose a reason for hiding this comment

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

Trailing spaces

CHANGELOG.md Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Mar 7, 2022

Code Climate has analyzed commit 80fe998 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 2

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 87.9% (-1.3% change).

View more on Code Climate.

@ajay-vel ajay-vel requested a review from micahlee March 7, 2022 16:32
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, thanks for the effort getting this wrapped up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants