-
-
Notifications
You must be signed in to change notification settings - Fork 383
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
feat: add linter support for step-level depends_on
existence
#4657
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks!
@@ -94,6 +94,9 @@ func (l *Linter) lintFile(config *WorkflowConfig) error { | |||
if err := l.lintContainers(config, "services"); err != nil { | |||
linterErr = multierr.Append(linterErr, err) | |||
} | |||
if err := l.lintDependsOn(config); err != nil { |
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.
Can you move this call into lintContainers
?
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'm a little bit hesitant because currently we only support depends_on
for steps, and it's unclear how dependencies across areas work (they should). Both of them make depends_on
check different from other lintContainers
check that's restricted to each area or even a single container.
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.
Well, you're just linting the containers. Yes, it's different for the areas, but can't you just add an if
to only run that for the steps area?
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.
Not sure if this is what you expect…?
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.
Yes, basically, that's what I mean.
But can you move the if
statemrnt to lintContainer
as well?
depends_on
existencedepends_on
existence
af44627
to
7bb8080
Compare
RT. This is useful as one could typo or forget to update the name everywhere.
Note that this PR is not implementing #1139, which is otherwise at workflow level.