Improve FAH create & delete #6726

inlined · 2024-01-30T02:27:59Z

Create now sets deployment-tool so we can track where users are coming from
Create now sets a rollout policy like the Console
Delete now works
Delete no longer makes buildId required

1. Create now sets deployment-tool so we can track where users are coming from 2. Create now sets a rollout policy like the Console 3. Delete now works 4. Delete no longer makes buildId required

codecov-commenter · 2024-01-30T03:09:42Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (c7fbe64) 54.19% compared to head (3f993de) 54.17%.

Files	Patch %	Lines
src/gcp/apphosting.ts	0.00%	7 Missing ⚠️
src/init/features/apphosting/index.ts	14.28%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6726      +/-   ##
==========================================
- Coverage   54.19%   54.17%   -0.02%     
==========================================
  Files         347      347              
  Lines       24154    24163       +9     
  Branches     4992     4994       +2     
==========================================
+ Hits        13090    13091       +1     
- Misses       9865     9873       +8     
  Partials     1199     1199

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bkendall · 2024-01-31T20:01:15Z

src/commands/apphosting-backends-delete.ts

@@ -47,21 +45,27 @@ export const command = new Command("apphosting:backends:delete")
      });
    }

+    let backend: apphosting.Backend;
+    if (backendId) {


Okay, this is odd. According to the approved proposal, the backend ID is supposed to be a required parameter, not a flag. That eliminates the need to pick a backend at all.

Irrespective of the proposal, do you like this better? It seemed odd to me that location could be browsed but backend couldn't. If you like this better we can push it through API council. As an EAP API, do you feel like it is OK to release and then get approval, or should I splice this into another CL until after approval?

I personally don't like it better, because I hate the prompt selector flow in a CLI 🤷‍♂️ It's not a wrong thing to do, but I'm assuming that there were product discussions that brought us to the API proposal (as that's a requirement at the top of the document) and that the pattern in the proposal was agreed upon by all the relevant stakeholders already. If you'd like to make a change to what was proposed there, starting back in the process, involving the right people, and getting an addendum would be appropriate.

That said, I thought the general pattern was to:

require backendID as a parameter in the command

default to looking at us-central1 as we 'traditionally' have

If someone is outside of us-central1, they get to include the --location flag.

Having this flow also makes it more complex. Sticking with the picker, you have to also consider what happens in non-interactive environment (like a CI system). As written, this would hang, I believe, which may be unexpected (instead of simply failing out).

🫡 I'll move the changes into another branch and bring up the idea in the devx room

(If it's chosen that we should keep the existing dialog, I agree that we need to check for nonInteractive)

bkendall · 2024-01-31T20:04:47Z

src/test/init/apphosting/index.spec.ts

    };

-    it("should createBackend", async () => {
+    it("should createBackend with rollout policy", async () => {


Nothing here tests to make sure the rollout policy was called. Please add the appropriate logic here to test for that

I may need to revert the label. I originally wrote the test to exercise this but realized it was for a smaller utility function and the larger flow is completely untested. I can either:

Move creation of the rollout policy into a utility function and test it. This would be a trivial whitebox test that is unlikely to catch anything though.

Add a new test that covers the overall flow of the entiere init process.

Cry that init functions are generally sparsely tested in this codebase.

You make the call and I'll follow.

Ah, yeah, this isn't actually testing doSetup. Probably just revert the test name then. init function coverage is kinda a known weak point.

bkendall · 2024-02-01T20:38:33Z

src/commands/apphosting-backends-delete.ts

@@ -47,21 +45,27 @@ export const command = new Command("apphosting:backends:delete")
      });
    }

+    let backend: apphosting.Backend;
+    if (backendId) {


I personally don't like it better, because I hate the prompt selector flow in a CLI 🤷‍♂️ It's not a wrong thing to do, but I'm assuming that there were product discussions that brought us to the API proposal (as that's a requirement at the top of the document) and that the pattern in the proposal was agreed upon by all the relevant stakeholders already. If you'd like to make a change to what was proposed there, starting back in the process, involving the right people, and getting an addendum would be appropriate.

That said, I thought the general pattern was to:

require backendID as a parameter in the command

default to looking at us-central1 as we 'traditionally' have

If someone is outside of us-central1, they get to include the --location flag.

Having this flow also makes it more complex. Sticking with the picker, you have to also consider what happens in non-interactive environment (like a CI system). As written, this would hang, I believe, which may be unexpected (instead of simply failing out).

bkendall · 2024-02-01T20:44:15Z

src/test/init/apphosting/index.spec.ts

    };

-    it("should createBackend", async () => {
+    it("should createBackend with rollout policy", async () => {


Ah, yeah, this isn't actually testing doSetup. Probably just revert the test name then. init function coverage is kinda a known weak point.

inlined · 2024-02-02T23:33:39Z

Controversial UX changes have been removed and test name has been reverted. PTAL.

bkendall · 2024-02-05T21:22:23Z

src/gcp/apphosting.ts

@@ -197,10 +197,20 @@ export interface RolloutPolicy {
  // end oneof trigger
  stages?: RolloutStage[];
  disabled?: boolean;
+
+  // TODO: This will be undefined if disabled is not true, right?


Yes, it should be.

Improve FAH create & delete

9766d8d

1. Create now sets deployment-tool so we can track where users are coming from 2. Create now sets a rollout policy like the Console 3. Delete now works 4. Delete no longer makes buildId required

inlined requested a review from bkendall January 30, 2024 02:27

inlined and others added 3 commits January 29, 2024 18:38

Fix test breakage where labels were wrong

4850bbd

Remove unused stub

399c31b

Merge branch 'master' into inlined.fah-rollouts

341e39a

bkendall requested changes Jan 31, 2024

View reviewed changes

inlined requested a review from bkendall February 1, 2024 16:56

bkendall requested changes Feb 1, 2024

View reviewed changes

inlined and others added 2 commits February 1, 2024 17:35

Revert unwanted changes

cb1546f

Merge branch 'master' into inlined.fah-rollouts

3f993de

inlined requested a review from bkendall February 2, 2024 23:33

bkendall approved these changes Feb 5, 2024

View reviewed changes

Merge branch 'master' into inlined.fah-rollouts

3e0d5d5

inlined enabled auto-merge (squash) February 5, 2024 22:59

inlined merged commit 1473c04 into master Feb 5, 2024
36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve FAH create & delete #6726

Improve FAH create & delete #6726

inlined commented Jan 30, 2024

codecov-commenter commented Jan 30, 2024 •

edited

bkendall Jan 31, 2024

inlined Feb 1, 2024

bkendall Feb 1, 2024

inlined Feb 2, 2024

inlined Feb 2, 2024

bkendall Jan 31, 2024

inlined Feb 1, 2024

bkendall Feb 1, 2024

bkendall Feb 1, 2024

bkendall Feb 1, 2024

inlined commented Feb 2, 2024

bkendall Feb 5, 2024

Improve FAH create & delete #6726

Improve FAH create & delete #6726

Conversation

inlined commented Jan 30, 2024

codecov-commenter commented Jan 30, 2024 • edited

Codecov Report

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

inlined commented Feb 2, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jan 30, 2024 •

edited