Skip to content
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

Workflows improvements (ENVs, Caches, Include/Exclude) #4

Merged
merged 1 commit into from
Apr 10, 2022

Conversation

ihostage
Copy link
Member

@ihostage ihostage commented Feb 3, 2022

Add custom env variables for single and matrix workflows.
Add custom cache and include/exclude for matrix workflow.

Add custom cache and include/exclude for matrix workflow.
@ihostage
Copy link
Member Author

ihostage commented Feb 3, 2022

Ping @octonato @mkurz for review 😄

Copy link
Contributor

@octonato octonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the value of tweaking the cache key and path, but not clear why we need the rest. Could you clarify?

Comment on lines +45 to +46
include: ${{ fromJson(inputs.include) }}
exclude: ${{ fromJson(inputs.exclude) }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the use case for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just to support an existed GA matrix behavior.
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-including-additional-values-in-combinations
It can be useful in the future when we will want to have a more special matrix strategy than the cartesian product of two array with Java and Scala versions.

Comment on lines +41 to +44
cmd: sbt "-Dvar1=$VAR_1" "-Dvar2=$VAR_2" ++$SCALA_VERSION test
env: |
VAR_1=value
VAR_2=value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that enough to just call sbt "-Dvar1=value" ++$SCALA_VERSION test?
Why do we need to set vars and let then be resolved?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, maybe it's not a good example. 😞
I'm trying to solve the restriction that in GA we can't set an ENV vars in parent workflow and then use theirs in reusable workflows 😞
Firstly, currently, the achievement of this is that if cmd is not a single line sbt command but some script, I don't need to set vars as arguments of this script. For example

cmd: "script/big-task.sh"
env: |
  VAR1=value
  VAR2=value

Secondly, when GA will support YAML Anchors (actions/runner#1182), we will try to use anchor with env vars in parent workflow and just add them to many jobs instead of copy&paste vars as now (see my draft for main repo playframework/playframework#11142). 🤞

Copy link
Contributor

@octonato octonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I have the impression that most of the things we are adding here cannot be used yet or we don't need yet.

I'm fine to merge, but maybe we should keep things at a minimal and only add new stuff when we need them.

@mkurz mkurz merged commit 15f8284 into playframework:main Apr 10, 2022
@ihostage ihostage deleted the sharable-actions branch April 11, 2022 06:47
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.

3 participants