Skip to content

feat(W-17752806): Pipeline creation must support the generation property - #3221

Merged
justinwilaby merged 6 commits into
mainfrom
jw/W-17752806
Mar 10, 2025
Merged

feat(W-17752806): Pipeline creation must support the generation property#3221
justinwilaby merged 6 commits into
mainfrom
jw/W-17752806

Conversation

@justinwilaby

@justinwilaby justinwilaby commented Feb 18, 2025

Copy link
Copy Markdown
Contributor

Adds the generation field to the API request.

To Test

  1. Test using a fir app. Make sure the pipeline creation succeeds.
    W-17752806

@justinwilaby
justinwilaby marked this pull request as ready for review February 18, 2025 16:08
@justinwilaby
justinwilaby requested a review from a team as a code owner February 18, 2025 16:08

@eablack eablack 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.

There are a few issues here:

  1. Happy path doesn't work. When I run the command, I get what appears to be a 500 error:
Screenshot 2025-02-18 at 12 31 10 PM
  1. I think we should reconsider how this behaves, maybe talk with @dsouza-anush about it. Right now, the pipelines create command requires an --app flag. Adding the --generation flag means someone can try to create a fir pipeline with a cedar app. The api blocks this, thankfully, but we should stop this from happening earlier. My recommendation is we do one of two things:
  • Keep --app as a required flag and just pass the generation of that app when we do the API call to create a pipeline. If we do this, we do not need to offer generation as a flag option. I think this is the simplest solution.
  • Make '--app' an optional flag and add --generation as a flag and let the user possibly create a pipeline without an app. We probably should check to make sure in the CLI that there isn't a conflict between the --generation value and the app's generation if the user does provide an app.

@justinwilaby

Copy link
Copy Markdown
Contributor Author

I am working with @bchen528 on this 500 error. It does seem to be a backend issue related to this line.

Originally, I thought this was because the fir app I was using to test was deployed using the BuildService and not git push heroku main.

I'm not really sure why this flag is necessary. The call succeeds and the pipeline API correctly infers the generation without this flag present. There are almost no details in the ticket on this so my assumption was this flag would allow both fir and cedar apps on the same pipeline potentially for migration from cedar to fir on the same pipeline.

@justinwilaby

Copy link
Copy Markdown
Contributor Author

Moving this to draft until then.

@justinwilaby
justinwilaby marked this pull request as draft February 19, 2025 16:07
@eablack

eablack commented Feb 27, 2025

Copy link
Copy Markdown
Contributor

I'm not really sure why this flag is necessary. The call succeeds and the pipeline API correctly infers the generation without this flag present. There are almost no details in the ticket on this so my assumption was this flag would allow both fir and cedar apps on the same pipeline potentially for migration from cedar to fir on the same pipeline.

this was discussed, but just want to document for clarity. a flag to the cli command is not necessary, because the cli can infer what generation the pipeline should be from the app supplied with the required --app flag. including the generation in the pipeline creation api request is necessary because the api is changing to require it.

@justinwilaby
justinwilaby marked this pull request as ready for review March 10, 2025 17:06
@justinwilaby

Copy link
Copy Markdown
Contributor Author

#1150 has been deployed to prod. Moving this to "ready"

@eablack eablack 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.

Works great! One minor comment to adjust the test such that the post expects the body to now include generation.

Comment thread packages/cli/test/unit/commands/pipelines/create.unit.test.ts Outdated
@justinwilaby
justinwilaby enabled auto-merge (squash) March 10, 2025 18:01
@justinwilaby
justinwilaby merged commit 779c8b4 into main Mar 10, 2025
@justinwilaby
justinwilaby deleted the jw/W-17752806 branch March 10, 2025 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants