fix(gocd): remove s4s, mattrobenolt-kube references from pipelines and code#7875
fix(gocd): remove s4s, mattrobenolt-kube references from pipelines and code#7875mwarkentin wants to merge 2 commits intomasterfrom
Conversation
…d code S4S (sentry4sentry) is no longer used. This removes: - s4s health check bash scripts - s4s_health_check functions from GoCD pipeline templates - Updates PIPELINE_FIRST_STEP to reference `us` instead of `s4s` - Cleans up stale s4s/mattrobenolt-kube references in comments Made-with: Cursor
| self.max_batch_size, | ||
| self.max_batch_time, | ||
| compute_batch_size, | ||
| // we need to enable this to deal with storages where we skip 100% of values, such as |
There was a problem hiding this comment.
Maybe this means we can update the code? Not sure if there are non-s4s places where this applied.
| if __name__ == "__main__": | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument("--pipeline", default="deploy-snuba-s4s") | ||
| parser.add_argument("--pipeline", default="deploy-snuba-us") |
There was a problem hiding this comment.
should this also be s4s2 here?
There was a problem hiding this comment.
@mchen-sentry I wasn't sure exactly what this was doing - if it's supposed to be the first stage of the pipeline then yes.
Co-authored-by: Michael Warkentin <mwarkentin@hey.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| elastic_profile_id: 'snuba', | ||
| environment_variables: { | ||
| PIPELINE_FIRST_STEP: 'deploy-snuba-py-s4s', | ||
| PIPELINE_FIRST_STEP: 'deploy-snuba-py-s4s2', |
There was a problem hiding this comment.
Bug: The PIPELINE_FIRST_STEP variable is set to deploy-snuba-py-s4s2, a non-existent pipeline, instead of the intended deploy-snuba-py-us. This will cause deployment checks to fail.
Severity: CRITICAL
Suggested Fix
Change the value of PIPELINE_FIRST_STEP from 'deploy-snuba-py-s4s2' to 'deploy-snuba-py-us'. A corresponding change should be made in snuba-rs.libsonnet to use 'deploy-snuba-rs-us'.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: gocd/templates/pipelines/snuba-py.libsonnet#L171
Potential issue: The `PIPELINE_FIRST_STEP` variable is incorrectly updated to
`deploy-snuba-py-s4s2` instead of the intended `deploy-snuba-py-us`. This variable is
used by the `check-migrations.sh` script during the `checks` stage of deployments. The
script attempts to fetch deployment history from the GoCD API for the specified
pipeline. Since the `deploy-snuba-py-s4s2` pipeline does not exist, the API call will
fail, causing the script to exit with an error. This failure will block all `snuba-py`
and `snuba-rs` deployments, preventing any new code from reaching production.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 05ff680. Configure here.
| if __name__ == "__main__": | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument("--pipeline", default="deploy-snuba-s4s") | ||
| parser.add_argument("--pipeline", default="deploy-snuba-us") |
There was a problem hiding this comment.
Default pipeline name likely references nonexistent pipeline
Medium Severity
The default pipeline was changed from deploy-snuba-s4s to deploy-snuba-us, but the GoCD pipeline configs in this repo only define names snuba-py and snuba-rs, which generate pipelines like deploy-snuba-py-{region} and deploy-snuba-rs-{region}. There's no evidence a deploy-snuba-us pipeline exists. Since PIPELINE_FIRST_STEP was updated to deploy-snuba-py-s4s2 and deploy-snuba-rs-s4s2, the reviewers also flagged that this default likely needs to reference an s4s2 pipeline instead.
Reviewed by Cursor Bugbot for commit 05ff680. Configure here.
2 participants
We killed s4s today, here's my attempt at getting claude to clean up references.. feel free to tear it apart.
Summary
PIPELINE_FIRST_STEPto referenceusinstead ofs4sin both snuba-py and snuba-rs pipelinesmattrobenolt-kubehostname in a trace log parsing example commentTest plan
check-migrations.shworks with the updatedPIPELINE_FIRST_STEPvaluesMade with Cursor