-
Notifications
You must be signed in to change notification settings - Fork 18
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
chore: fix specs run condition #488
Conversation
✅ Deploy Preview for api-clients-automation canceled.
|
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
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.
Nice !
.github/actions/setup/action.yml
Outdated
@@ -135,12 +135,12 @@ outputs: | |||
RUN_CTS: | |||
description: Determine if the `cts` jobs should run | |||
value: ${{ | |||
steps.spec-matrix.outputs.RUN_SPECS || |
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.
Where is RUN_SPECS
defined ?
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.
In the specs matrix step Compute specs matrix
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.
ok so it's always true, I think you can remove it
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.
you don't want to remove it ?
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.
Ah I thought you were talking about the RUN_CTS, which would always be true
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.
do you see anything else to remove here? 4a529f5
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.
RUN_CTS
is always true now also, we lost the ability to run just the CTS without generation the client but's that's fine, it's fast
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.
Looking good ! It's already a big simplification :)
🧭 What and Why
🎟 JIRA Ticket: -
Changes included:
As observed in #487, the specs are not running while changes are definitely here.
This PR fixes the run conditions of the
specs
andclients
jobs:run
output set per languageJS_
variables to match actual language nameMATRIX_DEPENDENCIES
definition to make it more dynamic🧪 Test
CI :D