Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Composite Run Steps ADR #554
Composite Run Steps ADR #554
Changes from 22 commits
edbe74a
185ef8c
d30db96
02b6823
91bb0a9
8e42609
45c1514
bc8488e
c712d5d
ef811a4
474f239
0741388
d4a1c00
ac9354f
0d8e0f0
a48fc79
02020f1
bcaed7a
d74a9ab
7e432a5
c5ddde9
dc63a51
225ef90
4170900
292dab3
45458e9
de80742
51f06d4
bba6d10
1caedf1
8ae6dd9
22c8c27
43cd477
82079ae
580cd08
c9eb5df
3ea4aed
5383305
4bc9ce4
d2843e8
b2199ac
9051dfc
c88a953
3166328
e3489c5
9664871
53da2f5
f9d6481
18124bd
1be703b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't think we support a step level needs: https://github.com/actions/runner/blob/master/src/Sdk/DTPipelines/workflow-v1.0.json
Is this a new addition as a part of this adr?
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.
let's consider starting without
env
at this level in the action.yml (extra complexity)also we should not allow the secrets context inside action.yml. Should pass secrets through.
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.
also should we consider not allowing
env
context inside action.ymle.g. can't do the following inside action.yml:
...even though explicily set
env
flow to the processes.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.
According to this logic, workflow authors won't be able to override environment variables.
On the one hand, I can see how it would be useful for an action author to want full control over their scope, to avoid collisions (I can't think of an example off-hand).
On the other, I can see it as useful for the action author to set a default, with the intention of it being overwritten (all our deploy scripts use the environment variable
STAGE
to control which environment to manage).A good compromise here might be to allow the use of
input
within the top levelenv
.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.
For additional clarity in the example, can we also explicitly say what happens when you specify
env
on the step itself.My assumption is that it will behave the same as above, though it could be a bit of a surprise to workflow authors when explicitly setting an env doesn't change what the action has set
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.
what about all the commands we have, ex:
::set-env::
will it only scope to the composite action, or still affect steps within the job?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 think following our logic that the workflow author shouldn't know the internal workings of the composite action, we should say that::set-env::
will only scope to the composite action.Update: users can use set-env in composite action that will set env variables for workflow.
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.
Given that actions can set environment variables for the job, it feel like
set-env
should also work the same way for composite actions.An usecase would be if someone wants to configure AWS credentials as part of their boilerplate for preparing a deploy.
This involves setting up environment variables.
The workaround here would be for the action author to output everything using
outputs
, and have the workflow author doset-env
themselves.. which is a fair amount of boilerplate, especially that to do it properly you need to handle escaping.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.
we may want to explicitly punt on
if
for now and move this to a separate ADR or appendixThere 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.
same for
continue-on-error
,timeout-minutes
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.
Agreed, I'd like the idea of starting with a more barebones implementation, and letting user feedback and scenarios drive how we can improve it going forward!
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.
the individual step will fail but the rest of the steps will run.
-> the rest of the steps will run base on condition.