search

Shopify / shipit-engine

Deployment coordination
https://shopify.engineering/introducing-shipit
MIT License
1.42k stars 144 forks source link

Add archive functionality to the stack update API call #1282

Closed mikailkarimi closed 2 years ago

mikailkarimi commented 2 years ago

🤷 What are you trying to accomplish?

We need API support for archive/lock a stack for our project. Modifying the API to be able to perform archive and lock functionalities on one stack. This includes unarchiving and unlocking functionalities as well.

Updated: As mentioned in PR review comment, there is a locks_controller we could directly leverage.

🛠️ How did you accomplish this and why did you choose this approach?

Modified the api/stacks_controller.rb to align with the current stacks_controller.rb. In order to avoid modification to the API behaviour, when the keys are not provided, we do not perform any modification to the stack. I added testing to the testing file to accommodate this change.

🎩 Tophat

In order to run tests in local, I installed redis.

brew install redis
redis-server

# another terminal tab
redis-cli
> ping

Then because I'm using M1, so I need to follow the instruction to install libpq before I can bundle install

brew install libpq
export PATH="/opt/homebrew/opt/libpq/bin:$PATH"
bundle install

Then all tests should pass.

xiaoxipang commented 2 years ago

Sorry, first time work on this repo so I have some basic questions: Do we need to squash all changes into one to keep history clean before merge? Do we need to release a new version for this API change and should I follow the semver, where the next version will be 0.38.0 with this public API change?

casperisfine commented 2 years ago

you've committed your redis database, you shouldn't.

casperisfine commented 2 years ago

Do we need to squash all changes into one to keep history clean before merge?

You can have multiple commits if they makes sense. Commits like "fix linter" etc should definitely be squashed.

Do we need to release a new version for this API change

No. Releases are handled separately. And you don't need to cut a release to get your change deployed on our Shipit instance.

xiaoxipang commented 2 years ago

Can you remove the redis database before merging?

Sorry, I felt so bad about it. I'll either stop the redis or select the files I really need to commit. 🤦‍♂️