-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: pipeline diff fails on v10 #3154
Conversation
c281d71
to
eb79f9f
Compare
eb79f9f
to
0281c0a
Compare
0281c0a
to
451bc36
Compare
451bc36
to
b99b58e
Compare
5593f70
to
9821489
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to split this PR into prioritizing a bugfix PR for the issue, and then a follow up PR with other refactors.
I agree in general that your implementation is correct. I would maintain things like introducing the Github interface types and remove those any
types. But changes to the Fir types file are so huge that even Github won't display them. I would suggest for the bug fix to restrict the changes to the relevant interfaces (Pipeline, PipelineCoupling, etc.), if they're needed, avoiding other changes because of the potential risk of effecting other commands that rely on the current state of those interfaces.
If we want to update the fir.d.ts
file by uploading another updated file autogenerated by the API schema converter tool, I would provide that as a separate PR with clear documentation on the interfaces that were updated and that are in use by any commands so we can test accordingly. I would love to rely on our current test suite to rule out any issues automatically, but we know there are commands that lack proper testing.
052196d
to
49d758a
Compare
Requested changes have been resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to approve because it looks like this fixes this bug. this pr would have benefited from a more explicit description of what was causing the bug and what specific change fixed it. there is a non-trivial amount of refactoring here, so it's a little hard to provide helpful feedback when it's unclear what the core issue is/was. we should probably revisit what our PR template looks like and how we can improve it so that we use it appropriately and put reviewers in the best position possible to provide feedback.
Fixes the problem with pipelines failing on v10
use
./bin/run pipelines:diff -a heroku-vscode-staging
to test - this pipeline and apps should be accessible to all heroku-dev-tools team members