[h2 deploy] Check for uncommited changes, output JSON in CI #1496

vincentezw · 2023-11-07T14:42:16Z

WHY are these changes introduced?

Make more clear when a folder contains uncommited changes prior to deploying.
Make it easier to obtain the deployment URL in CI scripts

WHAT is this pull request doing?

When using h2 deploy this PR checks if the Git folder is "clean" (i.e. has no uncommited changes). When uncommited changes are detected, the deployment is not allowed to proceed unless the new --force flag is used.

When there are uncommited changes, users can use the metadataDescription flag to update the description (commit message). If the flag is not used, a default "COMMIT_SHA with additional changes" is used.

For development purposes this PR also allows the deployment endpoint to be set using an environment variable.

We also needed to bump the version of cli-kit as the previous version contains a bug in the ensureGitClean function.

Lastly this PR lets the deploy command create a JSON output file in CI (and adds a no-json-output flag to prevent this). This can should be useful in CI environments where merchants want access to the deployment URL in subsequent workflow steps. Unfortunately the ink components do not lend themselves to output neatly to stderr, to send this output exclusively to stdout.

HOW to test your changes?

Post-merge steps

Checklist

I've read the Contributing Guidelines
I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've added a changeset if this PR contains user-facing or noteworthy changes
I've added tests to cover my changes
I've added or updated the documentation

frandiox

Thanks!
When do you think we can start showing deploy as a command?

packages/cli/src/commands/hydrogen/deploy.ts

frandiox · 2023-11-16T07:05:10Z

packages/cli/src/commands/hydrogen/dev.ts

@@ -249,6 +249,7 @@ async function runDev({
            type: 0,
            message:
              'MiniOxygen cannot start because the server bundle has not been generated.',
+            skipOclifErrorHandling: true,


Why is this needed here and in other commands? 🤔

This seems to be a required parameter in cli-kit 3.51.0 for FatalError https://github.com/Shopify/cli/blob/9d4c411176e13edcf352e4a4d5341aedaf1ee476/packages/cli-kit/src/public/node/error.ts#L27

Oh I see! I've seen there's a new renderError function. Maybe we should be using that instead of renderFatalError. Not a blocker for this PR though

I think renderFatalError is still more appropriate in this command, as it accepts an error and renders it appropriately, whereas renderError needs a headLine and body etc.

frandiox · 2023-11-16T07:07:23Z

packages/cli/src/lib/check-lockfile.ts

@@ -33,6 +33,7 @@ function missingLockfileWarning(shouldExit: boolean) {

 function multipleLockfilesWarning(lockfiles: Lockfile[], shouldExit: boolean) {
  const packageManagers = {
+    'bun.lockb': 'bun',


Lockfile now includes bun.lockb (here) so we'd get a Typescript error without it. If we don't want bun, we need to handle the case where the lockfile isn't a key in packageManagers by adding a check before accessing it.

Yes yes, I understand we need the changes, and I'm quite excited to support Bun actually 😄

ah ok I misunderstood the burn comment haha

.changeset/slimy-sloths-nail.md

frandiox · 2023-11-16T07:18:52Z

packages/cli/src/commands/hydrogen/deploy.ts

+    await ensureIsClean(path);
+  } catch (error) {
+    if (error instanceof GitDirectoryNotCleanError) {
+      isCleanGit = false;


Considering we are catching the error, is there any reason to use ensureIsClean() here instead of directly isClean() (returns true/false)?

There's a theoretical possibility the project dir is a non-git directory (i.e. doesn't have the .git folder). Whilst recommended we don't require a repository directory.
In that scenario isClean() throws a GitError (fatal: not a git repository) but we want to proceed, as telling the user to commit in a non-git directory doesn't make sense. With ensureIsClean we can check for a GitDirectoryNotCleanError which only appears when the project dir is a Git repository and has uncommitted changes.

What about flipping this around? Could we only check if it's clean if it's a git directory?

We could, but the only function that cli-kit exports to that extent is ensureInsideGitDirectory. Again that doesn't return a simple boolean, but a OutsideGitDirectoryError when outside of a Git folder, so I don't think that approach would be any cleaner.

frandiox · 2023-11-16T07:22:25Z

.changeset/slimy-sloths-nail.md

+'@shopify/cli-hydrogen': minor
+---
+
+The `deploy` command now displays an error if there are uncommited changes in a project's Git repository. If you'd like to go ahead with the deployment regardless, you can use the new `force` flag.


I guess it's not a big problem but it might be weird for users to read about h2 deploy in the changelog when the command is still hidden 🤔

I guess in a public repo, it is difficult to hide things. Hopefully we can go public soon.

👍🏻 I probably wouldn't add a changeset for something hidden

Yeah that's what I meant, we don't need to hide the code but it might be confusing if it's part of the release notes but still not usable 🤔
Should we then just remove this changelog for now?

vincentezw · 2023-11-16T10:44:25Z

Thanks! When do you think we can start showing deploy as a command?

Thanks for the review! I don't really have a better answer than "soon". We're still working on some things but I think we're almost there.

packages/cli/src/commands/hydrogen/deploy.test.ts

vincentezw force-pushed the vez/deploy-check-for-commit branch from 21df3c8 to 0fe25df Compare November 15, 2023 12:50

github-actions bot deployed to preview November 15, 2023 12:51 View deployment

vincentezw force-pushed the vez/deploy-check-for-commit branch from 0fe25df to 5be8092 Compare November 15, 2023 12:55

github-actions bot deployed to preview November 15, 2023 12:57 View deployment

vincentezw changed the title ~~[h2 deploy] Check for uncommited changes, add description and force flags~~ [h2 deploy] Check for uncommited changes, output JSON in CI Nov 15, 2023

Check for uncommited changes, write JSON output in CI

883fef0

vincentezw force-pushed the vez/deploy-check-for-commit branch from 5be8092 to 883fef0 Compare November 15, 2023 13:04

github-actions bot deployed to preview November 15, 2023 13:06 View deployment

github-actions bot deployed to preview November 15, 2023 15:18 View deployment

fix typecheck issues, allow bun as cli command

9258916

vincentezw force-pushed the vez/deploy-check-for-commit branch from 06791a0 to 9258916 Compare November 15, 2023 15:20

github-actions bot deployed to preview November 15, 2023 15:22 View deployment

vincentezw requested review from frandiox and graygilmore November 15, 2023 16:08

frandiox reviewed Nov 16, 2023

View reviewed changes

github-actions bot deployed to preview November 16, 2023 10:40 View deployment

PR feedback

38e8706

vincentezw force-pushed the vez/deploy-check-for-commit branch from 4e7bd92 to 38e8706 Compare November 16, 2023 11:03

github-actions bot deployed to preview November 16, 2023 11:04 View deployment

vincentezw marked this pull request as ready for review November 16, 2023 11:32

frandiox reviewed Nov 17, 2023

View reviewed changes

packages/cli/src/commands/hydrogen/deploy.test.ts Outdated Show resolved Hide resolved

simplify test, remove changelog

0b49695

github-actions bot deployed to preview November 17, 2023 10:48 View deployment

frandiox approved these changes Nov 17, 2023

View reviewed changes

frandiox merged commit 9ed789d into main Nov 17, 2023
9 checks passed

frandiox deleted the vez/deploy-check-for-commit branch November 17, 2023 11:38

shopify-github-actions-access bot mentioned this pull request Nov 17, 2023

[ci] release 2023-10 #1491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[h2 deploy] Check for uncommited changes, output JSON in CI #1496

[h2 deploy] Check for uncommited changes, output JSON in CI #1496

vincentezw commented Nov 7, 2023 •

edited

Loading

frandiox left a comment

frandiox Nov 16, 2023

vincentezw Nov 16, 2023

frandiox Nov 17, 2023

vincentezw Nov 17, 2023

frandiox Nov 16, 2023

vincentezw Nov 16, 2023

frandiox Nov 17, 2023

vincentezw Nov 17, 2023

frandiox Nov 16, 2023

vincentezw Nov 16, 2023 •

edited

Loading

graygilmore Nov 16, 2023

vincentezw Nov 17, 2023 •

edited

Loading

frandiox Nov 16, 2023

vincentezw Nov 16, 2023

graygilmore Nov 16, 2023

frandiox Nov 17, 2023

vincentezw Nov 17, 2023

vincentezw commented Nov 16, 2023

[h2 deploy] Check for uncommited changes, output JSON in CI #1496

[h2 deploy] Check for uncommited changes, output JSON in CI #1496

Conversation

vincentezw commented Nov 7, 2023 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

HOW to test your changes?

Post-merge steps

Checklist

frandiox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentezw Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentezw Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentezw commented Nov 16, 2023

vincentezw commented Nov 7, 2023 •

edited

Loading

vincentezw Nov 16, 2023 •

edited

Loading

vincentezw Nov 17, 2023 •

edited

Loading