[Proposal] Restructuring PullRequest #1093
Comments
Yeah I'm fine with that. That's actually a design decision I've come to regret a while ago. So I'm very much ok for at least a rename of
What's been publicly presented as "Merge Queue v2" is an entirely distinct project from |
Are you able to confirm that the intent is not for that project to supersede shipit-engine? Are they intended to be complementary or competing tools? Are there any thoughts of removing the existing merge queue functionality from shipit-engine in favour of that new project? |
I don't work on the project and have only a superficial understanding of the project, so.... But the internal code-name is
Nope, that won't happen. We might stop using it internally, but we have no reason to scrap it. |
indiebrain
added a commit
to powerhome/shipit-engine
that referenced
this issue
Aug 5, 2020
Implements the proposal of Shopify/shipit-engine issue Shopify#1093 by renaming the concept of the `Shipit::PullRequest` to `Shipit::MergeRequest`. Consideration was given to maintaining the name "PullRequest" where shipit-engine is interacting directly with the Github API - for example, in the `Shipit.github.api client`. In this way the `Shipit::MergeRequest` is related to Github Pull Requests, but not necessarily _of_ a Github Pull Request. The goal is to make way for a more direct modeling of a Github Pull Request in shipit-engine - `Shipit::PullRequest` - which like `Shipit::Commit` models the Github concept. The plan is to use this closer modeling of the Github Pull Request to finish preparations for submission of our "Review Stack" feature to `Shopify/shipit-engine`. References ---------- - Shopify#1093
indiebrain
added a commit
to powerhome/shipit-engine
that referenced
this issue
Aug 5, 2020
Implements the proposal of Shopify/shipit-engine issue Shopify#1093 by renaming the concept of the `Shipit::PullRequest` to `Shipit::MergeRequest`. Consideration was given to maintaining the name "PullRequest" where shipit-engine is interacting directly with the Github API - for example, in the `Shipit.github.api client`. In this way the `Shipit::MergeRequest` is related to Github Pull Requests, but not necessarily _of_ a Github Pull Request. The goal is to make way for a more direct modeling of a Github Pull Request in shipit-engine - `Shipit::PullRequest` - which like `Shipit::Commit` models the Github concept. The plan is to use this closer modeling of the Github Pull Request to finish preparations for submission of our "Review Stack" feature to `Shopify/shipit-engine`. Notes ------- This does include non-backward compatible HTTP API changes to shipit-engine - the `pull_requests` collection resources have been renamed to `merge_requests`. References ---------- - Shopify#1093
indiebrain
added a commit
to powerhome/shipit-engine
that referenced
this issue
Aug 6, 2020
Implements the proposal of Shopify/shipit-engine issue Shopify#1093 by renaming the concept of the `Shipit::PullRequest` to `Shipit::MergeRequest`. Consideration was given to maintaining the name "PullRequest" where shipit-engine is interacting directly with the Github API - for example, in the `Shipit.github.api client`. In this way the `Shipit::MergeRequest` is related to Github Pull Requests, but not necessarily _of_ a Github Pull Request. The goal is to make way for a more direct modeling of a Github Pull Request in shipit-engine - `Shipit::PullRequest` - which like `Shipit::Commit` models the Github concept. The plan is to use this closer modeling of the Github Pull Request to finish preparations for submission of our "Review Stack" feature to `Shopify/shipit-engine`. Notes ------- This does include non-backward compatible HTTP API changes to shipit-engine - the `pull_requests` collection resources have been renamed to `merge_requests`. References ---------- - Shopify#1093
indiebrain
added a commit
to powerhome/shipit-engine
that referenced
this issue
Aug 6, 2020
Implements the proposal of Shopify/shipit-engine issue Shopify#1093 by renaming the concept of the `Shipit::PullRequest` to `Shipit::MergeRequest`. Consideration was given to maintaining the name "PullRequest" where shipit-engine is interacting directly with the Github API - for example, in the `Shipit.github.api client`. In this way the `Shipit::MergeRequest` is related to Github Pull Requests, but not necessarily _of_ a Github Pull Request. The goal is to make way for a more direct modeling of a Github Pull Request in shipit-engine - `Shipit::PullRequest` - which like `Shipit::Commit` models the Github concept. The plan is to use this closer modeling of the Github Pull Request to finish preparations for submission of our "Review Stack" feature to `Shopify/shipit-engine`. Notes ------- This does include non-backward compatible HTTP API changes to shipit-engine - the `pull_requests` collection resources have been renamed to `merge_requests`. References ---------- - Shopify#1093
indiebrain
added a commit
to powerhome/shipit-engine
that referenced
this issue
Aug 6, 2020
* Use api responses directly instead of through last_response * Move exception handling * Don't set response if no data * Add mini_racer in development to not depend on node or other system wide JS engine * Preload Stack#repository by default to fix several N+1 * Fix mocha deprecations * Add a power user endpoint to list all stacks of a repo * Implement a stacks#update API * Fix api/stacks#update route * Save the stack before locking if only the spec has changed * Local dev fixes * Add dedicated view for tasks * Add rollback api * Add frozen string literal comment * Make the latest deployed reference branch opt-in Fix: Shopify#955 * Fix timeline page * Abort active deploy on rollback with force * Perform rollback after abort Use mechanism for triggering revert after abort to trigger a rollback to a specific deploy. * Update test to use until commit id rather than deploy * Update npm-lerna.md * Adapt merge status to new GitHub UI Github changed the width of their page which is why the "Add to merge queue" button looks out of place now. I removed the max-width constraint as this gave me the right results when playing around in the chrome web inspector. * Remove capitilisation from ShipIt repo listing * Update dependencies and test Ruby 2.7 * Workaround the active_model_serializer lookup bug Long story short the constant lookip semantic changed slightly in 2.7.0+ and 2.6.6+. Previously `Shipit::Shipit::Commit` would evaluate to `::Shipit:Commit`. Because of this, badly configured AMS would kinda work. Now it need to be more correct. * Update bundler on CI * Release 0.32.0 * rubocop formatting * changed stack query * Fix query and add test for an edge-case. * Disable the Style/PerlBackrefs cop * Avoid loading useless parts of Rails in development * Rename Shipit::PullRequest to Shipit::MergeRequest Implements the proposal of Shopify/shipit-engine issue Shopify#1093 by renaming the concept of the `Shipit::PullRequest` to `Shipit::MergeRequest`. Consideration was given to maintaining the name "PullRequest" where shipit-engine is interacting directly with the Github API - for example, in the `Shipit.github.api client`. In this way the `Shipit::MergeRequest` is related to Github Pull Requests, but not necessarily _of_ a Github Pull Request. The goal is to make way for a more direct modeling of a Github Pull Request in shipit-engine - `Shipit::PullRequest` - which like `Shipit::Commit` models the Github concept. The plan is to use this closer modeling of the Github Pull Request to finish preparations for submission of our "Review Stack" feature to `Shopify/shipit-engine`. Notes ------- This does include non-backward compatible HTTP API changes to shipit-engine - the `pull_requests` collection resources have been renamed to `merge_requests`. References ---------- - Shopify#1093 Co-authored-by: Jack Li <jack.li@shopify.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Darren Worrall <darren.worrall@shopify.com> Co-authored-by: Darren Worrall <DazWorrall@users.noreply.github.com> Co-authored-by: Kaelig Deloumeau-Prigent <kaelig@users.noreply.github.com> Co-authored-by: Robin Brandt <robin.brandt@shopify.com> Co-authored-by: Robin Brandt <robinbrandt@users.noreply.github.com> Co-authored-by: Alex Page <alex@alexpage.com.au> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> Co-authored-by: Ruidan Ji <ruidan.ji@shopify.com> Co-authored-by: Jonathan Geiger <jonathan.geiger@shopify.com> Co-authored-by: rdji123 <36777675+rdji123@users.noreply.github.com>
indiebrain
added a commit
to powerhome/shipit-engine
that referenced
this issue
Sep 1, 2020
* Use api responses directly instead of through last_response * Move exception handling * Don't set response if no data * Add mini_racer in development to not depend on node or other system wide JS engine * Preload Stack#repository by default to fix several N+1 * Fix mocha deprecations * Add a power user endpoint to list all stacks of a repo * Implement a stacks#update API * Fix api/stacks#update route * Save the stack before locking if only the spec has changed * Local dev fixes * Add dedicated view for tasks * Add rollback api * Add frozen string literal comment * Make the latest deployed reference branch opt-in Fix: Shopify#955 * Fix timeline page * Abort active deploy on rollback with force * Perform rollback after abort Use mechanism for triggering revert after abort to trigger a rollback to a specific deploy. * Update test to use until commit id rather than deploy * Update npm-lerna.md * Adapt merge status to new GitHub UI Github changed the width of their page which is why the "Add to merge queue" button looks out of place now. I removed the max-width constraint as this gave me the right results when playing around in the chrome web inspector. * Remove capitilisation from ShipIt repo listing * Update dependencies and test Ruby 2.7 * Workaround the active_model_serializer lookup bug Long story short the constant lookip semantic changed slightly in 2.7.0+ and 2.6.6+. Previously `Shipit::Shipit::Commit` would evaluate to `::Shipit:Commit`. Because of this, badly configured AMS would kinda work. Now it need to be more correct. * Update bundler on CI * Release 0.32.0 * rubocop formatting * changed stack query * Fix query and add test for an edge-case. * Disable the Style/PerlBackrefs cop * Avoid loading useless parts of Rails in development * Rename Shipit::PullRequest to Shipit::MergeRequest Implements the proposal of Shopify/shipit-engine issue Shopify#1093 by renaming the concept of the `Shipit::PullRequest` to `Shipit::MergeRequest`. Consideration was given to maintaining the name "PullRequest" where shipit-engine is interacting directly with the Github API - for example, in the `Shipit.github.api client`. In this way the `Shipit::MergeRequest` is related to Github Pull Requests, but not necessarily _of_ a Github Pull Request. The goal is to make way for a more direct modeling of a Github Pull Request in shipit-engine - `Shipit::PullRequest` - which like `Shipit::Commit` models the Github concept. The plan is to use this closer modeling of the Github Pull Request to finish preparations for submission of our "Review Stack" feature to `Shopify/shipit-engine`. Notes ------- This does include non-backward compatible HTTP API changes to shipit-engine - the `pull_requests` collection resources have been renamed to `merge_requests`. References ---------- - Shopify#1093 * Don't redirect on enabling emergency mode * Removes capitalization of environments A-la Shopify#1091 * Serialize rollback once aborted to value * Revert the pull_request -> merge_request renaming in CommitSerializer [ref Shopify#1095] * Merge shopify master (#64) * Don't redirect on enabling emergency mode * Removes capitalization of environments A-la Shopify#1091 * Serialize rollback once aborted to value * Revert the pull_request -> merge_request renaming in CommitSerializer [ref Shopify#1095] Co-authored-by: Jack Li <jack.li@shopify.com> Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Jack Li <jack.li@shopify.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Darren Worrall <darren.worrall@shopify.com> Co-authored-by: Darren Worrall <DazWorrall@users.noreply.github.com> Co-authored-by: Kaelig Deloumeau-Prigent <kaelig@users.noreply.github.com> Co-authored-by: Robin Brandt <robin.brandt@shopify.com> Co-authored-by: Robin Brandt <robinbrandt@users.noreply.github.com> Co-authored-by: Alex Page <alex@alexpage.com.au> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> Co-authored-by: Ruidan Ji <ruidan.ji@shopify.com> Co-authored-by: Jonathan Geiger <jonathan.geiger@shopify.com> Co-authored-by: rdji123 <36777675+rdji123@users.noreply.github.com> Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com>
This was referenced Sep 2, 2020
Merged
casperisfine
added a commit
that referenced
this issue
Oct 1, 2020
* Add Repository admin UI * Break repository fragment cache when stacks are added/removed * Add "provision pr stacks" config option to Repositories Adds a configuration option to Repositories which we can use to determine if, given a webhook event, we should (de)provision stacks when pull requests are opened/closed. * Add ability to configure per-pull-request provisioning behavior Allow repositories to configure per-Pull-Request auto-provisioning behavior. When "Dynamically provision stacks for Pull Requests" is enabled: The "Allow All" (default) beahavior will provision a stack for every pull request. The "Allow With Label" behavior will provision a stack for a pull request if, and only if, the pull request has a label matching the value provided by the Repository's "Provisioning label" attribute. The "Prevent With Label" behavior will provision stacks for all pull requests EXCEPT those with a label matching the value provided by the Repository's "Provisioning label" attribute. * Link from stacks back to Repository view * Exposes DEPLOY_URL to deployments * Makes archived stacks toggle work on repo page * Permit locking stacks with machine-readable reason (#8) This allows (efficiently) querying for stacks locked for this reason while retaining a user-friendly presentation. * Allow flagging stacks as auto-provisioned (#9) Permits distinguishing them from manually-provisioned stacks for the purpose of counting and limiting the number which are active at any one time. * Fix CSS typo * Add environment_url to deployment status events (#11) When creating a deployment status let's add the `environment_url` to the payload we send to GitHub. The idea is that this gets us whatever awesome features with which the GitHub API uses this value - automatic linking to the deployment environment in the PR. References ---------- - https://developer.github.com/changes/2016-04-06-deployment-and-deployment-status-enhancements/#link-to-a-live-deployment * Allow host application to define system-wide links to stack header (#15) Give the mounting application a way to define additional links which apply to stacks across the system rather than per-stack. * Merge upstream master from Shopify/shipit-engine (#18) * Release 0.30.0 * Update changelog with 0.30.0 version entry * Render shipit.yml links as a menu-list in the nav For stacks with more than one or two links the navigation bar starts to wrap links. We _might_ prevent this by rendering the links as a sub-list instead. * Update omniauth-github * Add ability to perform variable substitution to stack links ...when at least one task has been performed on the stack - IE when we have something we can use to grab the environment variables. This maintains the previous functionality of the `shipit.yml` to declare static links for stacks like: ```yml links: logs: https://logs.application.com ``` but also affords consumers the ability to define template strings in the `shipit.yaml` like: ```yml links: logs: https://logs.${ENVIRONMENT}.application.com ``` Which, if the stack's environment is set to `production` will render a link to `https://logs.production.application.com` in the stack's header. Additionally, allow host applications to add environment values to the stack. All stack env values are also exposed to the `Stack#links` method for variable substitution. The host applications can provide their own values to the stack's environment by intercepting messages dispatched to the `Stack#env` - for example, a host application might do something like this in an initializer: config/initializer/shipit.yml ```ruby ActiveSupport::Reloader.to_prepare do ... module EnvironmentExtensions def env super .merge( # ... environment values from host application ) end end require 'shipit/stack' Shipit::Stack.prepend(EnvironmentExtensions) end ``` Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Jose Luis Salas <josacar@users.noreply.github.com> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> * Assigns review request to stack (#19) * GithubSyncJob must run once before syncing pull requests (#21) Require at least one GithubSyncJob has been successful for a stack before refreshing its pull requests. When a pull requests is created before stack sync it will create a detatched Shipit::Commit record for its head ref. When GithubSyncJob runs afterward the Sidekiq/Shipit::GithubSyncJob perpetually emits an ActiveRecord::RecordNotUnique error becuase the GithubSyncJob attempts to create the PullRequst's head ref Shipit::Commit again in a non-detatched state and violates the index_commits_on_sha_and_stack_id uniqueness index. * Introduces PR event and emits it after every changes (#23) * Link review stacks to their PR on Github (#24) * Reload records to be created on Github This prevents us from duplicating data on Github. See #1042 (comment) * [GOG-150] Adding pre-condition check for uncached deploy spec (#26) * Adding pre-condition check for uncached deploy spec * Added commit onto DeploySpec stack * Added DeploySpec repo * Added check_deploy_spec stack * Adding test to make sure trigger_continuous_delivery returns when cached_deploy_spec config is empty * removing whitespace * Removed multiline for spec config. Added newline at eof * Removing deletion of canary tasks from deploy spec test * Breaking down trigger_continuous_delivery fn. Removed duplicated env declaration * can_trigger_continuous_delivery * Further refactored trigger_continuous_delivery * Making delivery condition fns private * [GOG-153] Prevent erroneously marking deployments as dead soon after they start (#27) * Prevent immediately marking newly started deployments as dead There appears to be a race condition between the time when the PerformTaskJob marks a task as "running" and when the newly added RemoveDeadTasks job attempts to reap dead tasks. In some cases a deployment is almost immediately marked as dead after it starts. We believe this is because there critical section in which PerformTaskJob marks a task as running and ReapDeadTaskJob is run in which newly a task can be transitioned to a "running" status, but not yet marked alive in which a newly created task will be considered a "dead task" by the ReapDeaTasks job. This changeset aims a two pronged approach at preventing this race. First it makes the ReapDeadTasksJob less aggressive in its consideration of tasks as "dead" - it will now only consider tasks in a "running" or "aborted" state which are missing an "alive" monitor AND are 5 at least minutes old. Second, when PerformTaskJob starts working a task the task will ping the "alive" monitor as soon as the task is marked as running. The implementation of reporting that tasks are "alive" is somewhat imperfect, in that it it doesn't happen until we attempt to capture command output. There are therefore several things which happen between starting the task and the first command happening, namely one or two checks that the git repo is already checked out and a wait on a cache lock, whose timeout is also 15 seconds. This means a large opportunity for the "alive" status to expire. We should probably ensure that the task is considered alive for a period long enough to span this (maybe 30 seconds rather than 15), as well as ping after each check of `fetched?`. References ---------- - [Task marked as running, but not yet alive](https://github.com/Shopify/shipit-engine/blob/d48ceb4ffc21afbdf2701ce8c3a8936268afc3d5/app/jobs/shipit/perform_task_job.rb#L18) - [Task first marked as alive](https://github.com/Shopify/shipit-engine/blob/d48ceb4ffc21afbdf2701ce8c3a8936268afc3d5/app/jobs/shipit/perform_task_job.rb#L81) * Fix return type of Stack#trigger_continuous_delivery (#29) In 8c6ad04 we accidentally changed the return types of this method when bailing out early due to stack readiness. Here we aim to restore the original "always nil" return type. References ---------- - powerhome@8c6ad04 - https://ci.powerhrg.com/job/powerhome/job/milano/job/PR-283/5/console * Sync pr-review-stacks with Shopify Master (#31) * Add SameSite middleware to Shipit This opt-in middleware allows users to configure Shipit to allow cross-site cookie requests, as may be required by the Shipit browser extension. * Rubocop fixes * Remove browser_sniffer dependency * Toggle middleware on boot * Add test to cover multiple cookies being set * Fix compatibility with Ruby 2.6.6 and 2.7.x There is a subtle change in constant resolution that was backported to 2.6.6 from the 2.7.x branch. * Reload records to be created on Github This prevents us from duplicating data on Github. See #1042 (comment) * [GOG-150] Adding pre-condition check for uncached deploy spec (#26) * Adding pre-condition check for uncached deploy spec * Added commit onto DeploySpec stack * Added DeploySpec repo * Added check_deploy_spec stack * Adding test to make sure trigger_continuous_delivery returns when cached_deploy_spec config is empty * removing whitespace * Removed multiline for spec config. Added newline at eof * Removing deletion of canary tasks from deploy spec test * Breaking down trigger_continuous_delivery fn. Removed duplicated env declaration * can_trigger_continuous_delivery * Further refactored trigger_continuous_delivery * Making delivery condition fns private * Fix return type of Stack#trigger_continuous_delivery (#29) In 8c6ad04 we accidentally changed the return types of this method when bailing out early due to stack readiness. Here we aim to restore the original "always nil" return type. References ---------- - powerhome@8c6ad04 - https://ci.powerhrg.com/job/powerhome/job/milano/job/PR-283/5/console * Restore Stack#env * Short cut many queries until config is cached * Adding config_empty? method on DeploySpec. Minor refactoring to use new method * Renaming config_empty? to blank? Co-authored-by: Alex Taylor <alex.taylor@clio.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com> Co-authored-by: Darren Worrall <DazWorrall@users.noreply.github.com> Co-authored-by: Aaron Kuehler <aaron.kuehler@powerhrg.com> Co-authored-by: Aaron Kuehler <aaron.kuehler@gmail.com> * [GOG-129] Adds pull request assignees to Ship It (#30) * Introduces pull request assignments to user * Stores the assigned users when refreshing the pr * Only add assignee if user is known * Serialize assignees on PR * Update app/models/shipit/pull_request.rb Co-Authored-By: Ben Langfeld <blangfeld@powerhrg.com> Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com> * Corrects Rubocop lint violations * Add provision_status state machine to review stacks (#33) In order to facilitate provisioning, provisioned-resource limits, etc. for review stacks we want to introduce a proper "provisioning status" to indicate the state of the Review Environment. Note, this is a response to design feedback form the upstream repository maintainers. References ---------- - https://nitro.powerhrg.com/runway/backlog_items/GOG-179 - #991 (comment) * Expose provisioning lifecycle hooks (#35) * Expose provisioning lifecycle hooks In order to make the Review Stack concept more widely applicable, and therefore ready for integration upstream, we want to make the provisioning / deprovisioning of Review Stacks more general and extensible. The idea here is that the host application could supply its own (de)provisioning routines - either per-repository, or as a global default - which the stack will use when the appropriate transition event occurs. For example, imagine a repository which deploys to a Kubernetes cluster. The host application could register a Kubernetes provisioning handler to take care of creating / destroying kubernetes resources for a stack: - Define a provisioning Handler ```ruby class KubernetesProvisioningHandler < Shipit::ProvisioningHandler::Base def up # allocate a namespace, copy resources, etc end def down # delete the namespace, etc. end end ``` - Register a default provisioning handler - IE how repositories which don't explicitly define a provisoining handler will be provisioned: ```ruby Shipit::ProvisioningHandler.register(:default, KubernetesProvisioningHandler) ``` - Or, define a provisionng handler for a specific repository: ```ruby Shipit::ProvisioningHandler.register('powerhome/nitro-web', KubernetesProvisioningHandler) ``` A default, no-op provisioning handler is provided when neither a repository level, nor host-application-default provisioning handler is provided. * Simplify provisioning state machine Add events for provision succes / failure. Previously I modeled these as states/nouns. After attempting to integrate with the host application it _feels_ more appropriate in this moment to model them as events/verbs with the simplified de/provisioned nouns. * Allow provisioning handler to be configured via shipit.yml (#36) * Allow provisioning handler to be configured via shipit.yml Notes ----- Requires PR #35 to merge before this can be considered for adoption. References ---------- - powerhome#35 - powerhome#35 (comment) * Fix typo inreview stack documentation Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com> * Whitelist ProvisioningHandlers Responding to PR feedback: - powerhome#36 (comment) * Make requests for unregistered ProvisioningHandlers more loud Instead of logging a registry miss and carrying on - making assumptions for the user - let's be a bit more loud to raise attention to the fact that an unregistered provisioning handler was requested. Responding to PR feedback: - powerhome@84ec9ee#r40243419 * Surface unregistered provisioning error to end users When attempting to fetch an unregistered provisioning handler, surface this fact to the end user via the stack's lock. * Update docs around unregistered provisioning handler usage We don't throw an exception anymore, instead we halt the state machine and lock the stack. Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com> * Add ReviewStack into shipit engine (#37) * Add ReviewStack into shipit engine Makes ReviewStacks a 1st class citizen of shipit-engine. Additionally serves as a place to start pulling the review stack functionality from our host application down into shipit-engine. We've not pulled functionality from the host application into this body of work to minimize the risk to our existing users - IE let's tinker with the engine while the car's running down the highway, but let's leave the wheels alone for now. That is to say, there is much more work coming. * Fix review stack settings pages When accessing the review stack's setting page the ActiveRecord "record identification" system takes over and attempts to route form submits to `review_stack_path`. At this time there are no specific forms for `review_stacks` so we don't need this level of helpfulness from rails. Rather than imply attributes about the forms from the model, for now, lets be explicit about what we want. * Fix review stack routing (#38) * Fix review stack deployment paths * Fix review stack task lookup * Fix review stack tasks path * Rename provision_pr_stacks to review_stacks_enabled (#39) * Renaming provision_pr_stacks to review_stacks_enabled * Migration to rename provision_pr_stacks column to review_stacks_enabled * Remove foreign keys added from migrations. Alter test wording to reflect new column name * Revert changes that came from migrations * Add optional #provision? predicate to ProvisioningHandler (#40) * Add optional #provision? predicate to ProvisioningHandler In this way, custom ProvisioningHandlers can perform additional checks to ensure that ample resources are available to perform the provisioning process as a precondition to provisioning. * Remove auto_provisioned from Stacks (#42) * Remove references to auto_provisioned * Add migration to remove auto_provisioned column from stacks * Put milano working branch in sync with upstream master (#41) * Use api responses directly instead of through last_response * Move exception handling * Don't set response if no data * Add mini_racer in development to not depend on node or other system wide JS engine * Preload Stack#repository by default to fix several N+1 * Fix mocha deprecations * Add a power user endpoint to list all stacks of a repo * Implement a stacks#update API * Fix api/stacks#update route * Save the stack before locking if only the spec has changed * Local dev fixes * Add dedicated view for tasks * Add rollback api * Add frozen string literal comment * Make the latest deployed reference branch opt-in Fix: #955 * Fix timeline page * Abort active deploy on rollback with force * Perform rollback after abort Use mechanism for triggering revert after abort to trigger a rollback to a specific deploy. * Update test to use until commit id rather than deploy * Update npm-lerna.md * Adapt merge status to new GitHub UI Github changed the width of their page which is why the "Add to merge queue" button looks out of place now. I removed the max-width constraint as this gave me the right results when playing around in the chrome web inspector. * Remove capitilisation from ShipIt repo listing * Fix merge conflict issue Co-authored-by: Jack Li <jack.li@shopify.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Darren Worrall <darren.worrall@shopify.com> Co-authored-by: Darren Worrall <DazWorrall@users.noreply.github.com> Co-authored-by: Kaelig Deloumeau-Prigent <kaelig@users.noreply.github.com> Co-authored-by: Robin Brandt <robin.brandt@shopify.com> Co-authored-by: Robin Brandt <robinbrandt@users.noreply.github.com> Co-authored-by: Alex Page <alex@alexpage.com.au> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> * Move review stack resource allocation queue into shipit-engine (#43) * Sync working branch with upstream master (#44) * Use api responses directly instead of through last_response * Move exception handling * Don't set response if no data * Add mini_racer in development to not depend on node or other system wide JS engine * Preload Stack#repository by default to fix several N+1 * Fix mocha deprecations * Add a power user endpoint to list all stacks of a repo * Implement a stacks#update API * Fix api/stacks#update route * Save the stack before locking if only the spec has changed * Local dev fixes * Add dedicated view for tasks * Add rollback api * Add frozen string literal comment * Make the latest deployed reference branch opt-in Fix: #955 * Fix timeline page * Abort active deploy on rollback with force * Perform rollback after abort Use mechanism for triggering revert after abort to trigger a rollback to a specific deploy. * Update test to use until commit id rather than deploy * Update npm-lerna.md * Adapt merge status to new GitHub UI Github changed the width of their page which is why the "Add to merge queue" button looks out of place now. I removed the max-width constraint as this gave me the right results when playing around in the chrome web inspector. * Remove capitilisation from ShipIt repo listing * Update dependencies and test Ruby 2.7 * Workaround the active_model_serializer lookup bug Long story short the constant lookip semantic changed slightly in 2.7.0+ and 2.6.6+. Previously `Shipit::Shipit::Commit` would evaluate to `::Shipit:Commit`. Because of this, badly configured AMS would kinda work. Now it need to be more correct. * Workaround the active_model_serializer lookup bug Long story short the constant lookip semantic changed slightly in 2.7.0+ and 2.6.6+. Previously `Shipit::Shipit::Commit` would evaluate to `::Shipit:Commit`. Because of this, badly configured AMS would kinda work. Now it need to be more correct. * Update bundler on CI * Release 0.32.0 * Make PullRequestAssignment also be based on Record * Fix bad merge conflict resolution Removes duplicated code Co-authored-by: Jack Li <jack.li@shopify.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Darren Worrall <darren.worrall@shopify.com> Co-authored-by: Darren Worrall <DazWorrall@users.noreply.github.com> Co-authored-by: Kaelig Deloumeau-Prigent <kaelig@users.noreply.github.com> Co-authored-by: Robin Brandt <robin.brandt@shopify.com> Co-authored-by: Robin Brandt <robinbrandt@users.noreply.github.com> Co-authored-by: Alex Page <alex@alexpage.com.au> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> * Implement cleanup routine (#45) * Implement cleanup routine on shipit * Tests clearing stale caches logic * Fix tests and add extra coverage * Uses review stack model present on shipit * Adds enqueued concept to stack (#46) * Adds enqueued concept to stack The idea is to use this attributed instead of the lock reason to tell if a stack is waiting on resources so it can be provisioned. * Moves enqueue logic to review stack * Remove from queue should set enqueued false * Renames enqueued to awaiting_provision Enqueued can be confusing as it does not indicate what the stack is being enqueued for. The rename is so it easier to reason about what this attribute really is. * Also renames migration file and class * Merge upstream master to receive pull request rename (#49) * Use api responses directly instead of through last_response * Move exception handling * Don't set response if no data * Add mini_racer in development to not depend on node or other system wide JS engine * Preload Stack#repository by default to fix several N+1 * Fix mocha deprecations * Add a power user endpoint to list all stacks of a repo * Implement a stacks#update API * Fix api/stacks#update route * Save the stack before locking if only the spec has changed * Local dev fixes * Add dedicated view for tasks * Add rollback api * Add frozen string literal comment * Make the latest deployed reference branch opt-in Fix: #955 * Fix timeline page * Abort active deploy on rollback with force * Perform rollback after abort Use mechanism for triggering revert after abort to trigger a rollback to a specific deploy. * Update test to use until commit id rather than deploy * Update npm-lerna.md * Adapt merge status to new GitHub UI Github changed the width of their page which is why the "Add to merge queue" button looks out of place now. I removed the max-width constraint as this gave me the right results when playing around in the chrome web inspector. * Remove capitilisation from ShipIt repo listing * Update dependencies and test Ruby 2.7 * Workaround the active_model_serializer lookup bug Long story short the constant lookip semantic changed slightly in 2.7.0+ and 2.6.6+. Previously `Shipit::Shipit::Commit` would evaluate to `::Shipit:Commit`. Because of this, badly configured AMS would kinda work. Now it need to be more correct. * Update bundler on CI * Release 0.32.0 * rubocop formatting * changed stack query * Fix query and add test for an edge-case. * Disable the Style/PerlBackrefs cop * Avoid loading useless parts of Rails in development * Rename Shipit::PullRequest to Shipit::MergeRequest Implements the proposal of Shopify/shipit-engine issue #1093 by renaming the concept of the `Shipit::PullRequest` to `Shipit::MergeRequest`. Consideration was given to maintaining the name "PullRequest" where shipit-engine is interacting directly with the Github API - for example, in the `Shipit.github.api client`. In this way the `Shipit::MergeRequest` is related to Github Pull Requests, but not necessarily _of_ a Github Pull Request. The goal is to make way for a more direct modeling of a Github Pull Request in shipit-engine - `Shipit::PullRequest` - which like `Shipit::Commit` models the Github concept. The plan is to use this closer modeling of the Github Pull Request to finish preparations for submission of our "Review Stack" feature to `Shopify/shipit-engine`. Notes ------- This does include non-backward compatible HTTP API changes to shipit-engine - the `pull_requests` collection resources have been renamed to `merge_requests`. References ---------- - #1093 Co-authored-by: Jack Li <jack.li@shopify.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Darren Worrall <darren.worrall@shopify.com> Co-authored-by: Darren Worrall <DazWorrall@users.noreply.github.com> Co-authored-by: Kaelig Deloumeau-Prigent <kaelig@users.noreply.github.com> Co-authored-by: Robin Brandt <robin.brandt@shopify.com> Co-authored-by: Robin Brandt <robinbrandt@users.noreply.github.com> Co-authored-by: Alex Page <alex@alexpage.com.au> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> Co-authored-by: Ruidan Ji <ruidan.ji@shopify.com> Co-authored-by: Jonathan Geiger <jonathan.geiger@shopify.com> Co-authored-by: rdji123 <36777675+rdji123@users.noreply.github.com> * Makes awaiting provision stacks not deployable (#48) * Revert "Merge upstream master to receive pull request rename (#49)" (#50) This reverts commit e4ec9d8. * Revert "Revert "Merge upstream master to receive pull request rename (#49)" (#50)" (#51) This reverts commit e6213b2. * Emit review stack events (#52) * Move pull request webhooks to shipit engine (#54) * Rename pull_request_assignments to merge_request_assignments * Move NullRepository to shipit-engine * Move review stack webhook adapter to shipit-engine * Move PullRequestOpenedHandler to shipit-engine * Move PullRequestClosedHandler to shipit-engine * Move PullRequestReopenedHandler to shipit-engine * Add Shipit::PullRequest * Move PullRequestEditedHandler to shipit-engine * Move PullRequestAssignedHandler to shipit-engine * Transform old review_request MergeRequests into PullRequests * Make Shipit::Labels and PullRequest label associations a real thing * Move PullRequestLabeledHandler to shipit-engine * Move PullRequestUnlabeledHandler to shipit-engine * Fix lint * Move PullRequestLabelCapturingHandler to shipit-engine * Add PullRequestLabels to stack task/deploy environment * Remove merge_request assign_to_stack method * Add pull_request serializer (#55) * Remove review_request flag from MergeRequest (#57) "review requests" are now Proper Shipit::PullRequests * Fixes ActionView::Template::Error undefined method `pull_requset' (#58) References ---------- - https://sentry.io/organizations/power-home-remodeling-group/issues/1859024086/?project=1833592&query=is%3Aunresolved&statsPeriod=1h * Add head commit to pull requests (#56) We use this in our host application to determine if the currenty deployment would put the Review Stack in a "current with the head of the pull request" state - if so, we send notifications to the user who opened the pull request indicating that their instance is "ready". This was missed in the original PullRequest implementation. * Revert "Add head commit to pull requests (#56)" (#59) This reverts commit fbd5154. * Capture head commit for pull requsets (#60) * Revert "Revert "Add head commit to pull requests (#56)" (#59)" This reverts commit b914d6b. * Emit PullRequest hooks * Prevent updating labels for yet-to-be-created pull_requests When labels are applied at the time of PullRequest creation, sometimes the PullRequest Labeled event arrives to shipit before the PullRequest is created. In this case, we want to drop the "labeld" message because the pull request doesn't exist, AND we'll apply the labels via the pull request creation anyway. * Ensure stack has pull_request before PR emits events * Find out when things go wrong (#61) Because I won't look as good as Aaron without my hair. * Revert "Capture head commit for pull requsets (#60)" (#62) This reverts commit dbf6f7d. * Revert "Revert "Capture head commit for pull requsets (#60)" (#62)" (#63) This reverts commit f49e0ee. * Merge shopify master into pr review stacks branch (#65) * Use api responses directly instead of through last_response * Move exception handling * Don't set response if no data * Add mini_racer in development to not depend on node or other system wide JS engine * Preload Stack#repository by default to fix several N+1 * Fix mocha deprecations * Add a power user endpoint to list all stacks of a repo * Implement a stacks#update API * Fix api/stacks#update route * Save the stack before locking if only the spec has changed * Local dev fixes * Add dedicated view for tasks * Add rollback api * Add frozen string literal comment * Make the latest deployed reference branch opt-in Fix: #955 * Fix timeline page * Abort active deploy on rollback with force * Perform rollback after abort Use mechanism for triggering revert after abort to trigger a rollback to a specific deploy. * Update test to use until commit id rather than deploy * Update npm-lerna.md * Adapt merge status to new GitHub UI Github changed the width of their page which is why the "Add to merge queue" button looks out of place now. I removed the max-width constraint as this gave me the right results when playing around in the chrome web inspector. * Remove capitilisation from ShipIt repo listing * Update dependencies and test Ruby 2.7 * Workaround the active_model_serializer lookup bug Long story short the constant lookip semantic changed slightly in 2.7.0+ and 2.6.6+. Previously `Shipit::Shipit::Commit` would evaluate to `::Shipit:Commit`. Because of this, badly configured AMS would kinda work. Now it need to be more correct. * Update bundler on CI * Release 0.32.0 * rubocop formatting * changed stack query * Fix query and add test for an edge-case. * Disable the Style/PerlBackrefs cop * Avoid loading useless parts of Rails in development * Rename Shipit::PullRequest to Shipit::MergeRequest Implements the proposal of Shopify/shipit-engine issue #1093 by renaming the concept of the `Shipit::PullRequest` to `Shipit::MergeRequest`. Consideration was given to maintaining the name "PullRequest" where shipit-engine is interacting directly with the Github API - for example, in the `Shipit.github.api client`. In this way the `Shipit::MergeRequest` is related to Github Pull Requests, but not necessarily _of_ a Github Pull Request. The goal is to make way for a more direct modeling of a Github Pull Request in shipit-engine - `Shipit::PullRequest` - which like `Shipit::Commit` models the Github concept. The plan is to use this closer modeling of the Github Pull Request to finish preparations for submission of our "Review Stack" feature to `Shopify/shipit-engine`. Notes ------- This does include non-backward compatible HTTP API changes to shipit-engine - the `pull_requests` collection resources have been renamed to `merge_requests`. References ---------- - #1093 * Don't redirect on enabling emergency mode * Removes capitalization of environments A-la #1091 * Serialize rollback once aborted to value * Revert the pull_request -> merge_request renaming in CommitSerializer [ref #1095] * Merge shopify master (#64) * Don't redirect on enabling emergency mode * Removes capitalization of environments A-la #1091 * Serialize rollback once aborted to value * Revert the pull_request -> merge_request renaming in CommitSerializer [ref #1095] Co-authored-by: Jack Li <jack.li@shopify.com> Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Jack Li <jack.li@shopify.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Darren Worrall <darren.worrall@shopify.com> Co-authored-by: Darren Worrall <DazWorrall@users.noreply.github.com> Co-authored-by: Kaelig Deloumeau-Prigent <kaelig@users.noreply.github.com> Co-authored-by: Robin Brandt <robin.brandt@shopify.com> Co-authored-by: Robin Brandt <robinbrandt@users.noreply.github.com> Co-authored-by: Alex Page <alex@alexpage.com.au> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> Co-authored-by: Ruidan Ji <ruidan.ji@shopify.com> Co-authored-by: Jonathan Geiger <jonathan.geiger@shopify.com> Co-authored-by: rdji123 <36777675+rdji123@users.noreply.github.com> Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com> * Revert "Merge shopify master into pr review stacks branch (#65)" (#66) This reverts commit 21e7540. * Remove merge_request user and assignees (#69) * Remove stacks lock_reason_code (#70) * Fix Revie Stack documentation typos Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com> * Fixup merge 56d0976 (#73) * Fix commit serializer The upstream did just this in b94d49c. I think git missed this in this - 56d0976 - merge from the upstream. References ---------- - powerhome@b94d49c - powerhome@56d0976 * Fix stack default_scope * Fix seeds * Fix stack_test verbage * Remove no-op migrations (#72) Over the course of implementing and evolving the ReviewStack feature of shipit-engine we've evolved how we model the data. This cleans up that data modeling history to present a clear statement of intended changes to the existing schema of shipit-engine. This snapshot of the end product seems more appropriate for inclusion in the upstream PR of ReviewStacks rather than inclusion of every mis-step along the way. This was tested by checking out the shopify/master branch to setup the local database, then checking this branch out and running the migrations. The fact that the schema.rb was only impacted by reverting a change we didn't really want to make to the upstream schema gives me confidence that this chain of migrations is functionally equivalent to what was in place before. * Compress review stack feature flag migrations (#74) We _should_ create this feature toggle column with the appropriate name from the beginning in the upstream PR. Missed this one in 9d2ad93 References ---------- - powerhome@9d2ad93 * Atomically create Review Stacks and their Pull Requests (#75) Last evening we observed a Review Stack was created without a Pull Request. It is our intent that EVERY Review Stack should have an associated Pull Request. We suspect that the call to `PullRequest#update!` failed. Since creating the ReviewStack and PullRequest are flushed to the database independently, we ended up with a ReviewStack that was missing its PullRequest. This change attempts to make the creation of the Review Stack and its Pull Request atomic - ensuring that any Review Stack that is created will have captured its Pull Request. * Change provisioning_behavior to string enum * Document the relevance of throw(:halt) in provisoining handler * Add review stacks changes (#76) * No need to path JS String * Fix typos and improve style in repository settings page * Get rid of an unecessary .presence call * Remove MergeRequestAssigment model * Refactor ProvisioningHandler to be less defensive * Lint attr_accessor * Fix duplicate html ID in repo settings.html Co-authored-by: Jean Boussier <jean.boussier@gmail.com> * Store labels directly on PullRequests * Collapse migrations to one per table Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com> Co-authored-by: Jean Boussier <jean.boussier@gmail.com> Co-authored-by: Jose Luis Salas <josacar@users.noreply.github.com> Co-authored-by: Jean byroot Boussier <jean.boussier+github@shopify.com> Co-authored-by: Vinicius Gama <viniciusgama@users.noreply.github.com> Co-authored-by: Ryan Rodriguez <62253265+ryanmrodriguez@users.noreply.github.com> Co-authored-by: Alex Taylor <alex.taylor@clio.com> Co-authored-by: Darren Worrall <DazWorrall@users.noreply.github.com> Co-authored-by: Jack Li <jack.li@shopify.com> Co-authored-by: Darren Worrall <darren.worrall@shopify.com> Co-authored-by: Kaelig Deloumeau-Prigent <kaelig@users.noreply.github.com> Co-authored-by: Robin Brandt <robin.brandt@shopify.com> Co-authored-by: Robin Brandt <robinbrandt@users.noreply.github.com> Co-authored-by: Alex Page <alex@alexpage.com.au> Co-authored-by: Ruidan Ji <ruidan.ji@shopify.com> Co-authored-by: Jonathan Geiger <jonathan.geiger@shopify.com> Co-authored-by: rdji123 <36777675+rdji123@users.noreply.github.com>
|
This was implemented here #1093 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As an ongoing effort on our part to deliver Review Stacks into Shipit, we have a couple of decisions to make pertaining the structure of our changes. We would like to know your thoughts on the subject matter.
What are we trying to do?
Deliver a Review Stack feature into Shipit.
How are we implementing this?
We extend the Stack model, and rely on the creation and synchronization of Pull Request objects. This is kicked off with custom handlers.
What issues are we facing?
In order to tighten up the lifecycle of Review Stacks, we believe they should be owned by Pull Requests. However, within the current codebase, we feel that a PullRequest doesn't accurately reflect what we are meant to be modeling (a traditional Github Pull Request). Currently the PullRequest class is really only used for a merge queue. So that leads to a few decisions that need to be made before we proceed.
We believe that PullRequest should be split into "MergeRequest" and "PullRequest" to more accurately reflect what each is meant to be. MergeRequest would fill the role of the current PullRequest, and the new PullRequest class would take on modeling a traditional GitHub pull request in the same lightweight fashion as things like Commit, and would be used in our Review Stacks feature. How would you feel about this change? Does it make sense to you? Any other insights?
We have read about Merge Queue v2, and we are wondering how this would affect the merge queue v1 feature in Shipit. Is this something you guys can see being completely removed from Shipit in favor of using merge queue v2 (assuming it would be open-sourced)?
The text was updated successfully, but these errors were encountered: