Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feature/release: update sg release cut to automate stitch graph gen and release branch creation#63794

Merged
DaedalusG merged 15 commits into
mainfrom
al/rel-253/stitched-migration-graph-automation
Jul 19, 2024
Merged

feature/release: update sg release cut to automate stitch graph gen and release branch creation#63794
DaedalusG merged 15 commits into
mainfrom
al/rel-253/stitched-migration-graph-automation

Conversation

@DaedalusG

@DaedalusG DaedalusG commented Jul 11, 2024

Copy link
Copy Markdown
Contributor

Resolve: https://linear.app/sourcegraph/issue/REL-253/bug-automate-the-stitched-migration-graph-bazel-archive-generation

Test plan

Tested by running go run ./dev/sg release cut --version “5.7.0"

Changelog

@cla-bot cla-bot Bot added the cla-signed label Jul 11, 2024
@DaedalusG DaedalusG requested a review from jhchabran July 11, 2024 19:15
@Chickensoupwithrice

Chickensoupwithrice commented Jul 11, 2024

Copy link
Copy Markdown
Contributor

I'm still not sure how to work around the branch protection on 5.7.x (for example) and what actually needs to happen for creation to be successful?

Asides from that the rest of the automation works. It creates a git archive, and correctly changes the const maxVersionNumber.

@Chickensoupwithrice Chickensoupwithrice marked this pull request as ready for review July 11, 2024 22:40

@Chickensoupwithrice Chickensoupwithrice left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🍐'd on this

Comment on lines +175 to +181
func genStitchMigrationGraph(ctx context.Context) error {
err := run.Cmd(ctx, "sg", "bazel", "run", "//internal/database/migration/shared:write_stitched_migration_graph").Run().Wait()
if err != nil {
return errors.Wrap(err, "Could not run stitch migration generator")
}
return nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels a bit silly for me to do? Maybe I should call what sg bazel run calls with this argument?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd say this is fine. We do this in a number of other places as well. 👍

Comment on lines +175 to +181
func genStitchMigrationGraph(ctx context.Context) error {
err := run.Cmd(ctx, "sg", "bazel", "run", "//internal/database/migration/shared:write_stitched_migration_graph").Run().Wait()
if err != nil {
return errors.Wrap(err, "Could not run stitch migration generator")
}
return nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd say this is fine. We do this in a number of other places as well. 👍

if err != nil {
return errors.Wrap(err, "Could not create git archive")
}
err = run.Cmd(ctx, "CLOUDSDK_CORE_PROJECT=\"sourcegraph-ci\"", "gsutil", "cp", fmt.Sprintf("migrations-%s", newVersion), "gs://schemas-migrations/migrations/").Run().Wait()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should verify if the user running this has access to the GCP project and bucket before starting this, if we don't already. Perhaps in --pretend like the other checks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ugh, after much searching and looking around I ended up with gsutil ls gs://bucket/schemas-migrations which actually checks the wrong permission (storage.objects.list instead of storage.objects.create), which shouldn't be a problem based on my reading of Uniform bucket policy only to realize we don't have a --pretend flag for sg release cut 😮‍💨

@Chickensoupwithrice Chickensoupwithrice changed the title release/feature: update sg release cut to automate stitch graph gen and release branch creation feature/release: update sg release cut to automate stitch graph gen and release branch creation Jul 15, 2024
@DaedalusG DaedalusG merged commit 43df26f into main Jul 19, 2024
@DaedalusG DaedalusG deleted the al/rel-253/stitched-migration-graph-automation branch July 19, 2024 06:15
@DaedalusG DaedalusG restored the al/rel-253/stitched-migration-graph-automation branch July 23, 2024 01:16
@DaedalusG

Copy link
Copy Markdown
Contributor Author

@Chickensoupwithrice @jdpleiness Sorry I lost track of this one and thought it was good to merge, we probably want to come back to this. Anything we should break out into issues?

  • branch protections we're doing for allowing backports anyway
  • --pretend flag? Should we? In the other commands --pretend basically just verifies steps of the release.yaml def are invoked correctly. idk

@Chickensoupwithrice

Copy link
Copy Markdown
Contributor

For the --pretend flag we want it primarily to check that the permissions for GCP are set correctly, but even if they're wrong it's not too bad (the artifact still gets generated, it just doesn't get uploaded). Maybe at the very least we should have a nicer error message, include an entitle request form, and the command to run if it failed out due to permissions?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants