-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add a test workflow for rust projects #495
Conversation
* Otherwise just use the constructor. | ||
*/ | ||
static addToProject(project: GitHubProject, options: WithRustTestWorkflow) { | ||
const hasRustTestWorkflow = options.hasRustTestWorkflow ?? false |
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.
@quesabe do you think we can move this check to the parent? It doesn't feel right to me to be honest to let a nested construct to decide if it should be enabled in the caller.
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.
That's the nature of GitHub workflows - only the root/.github
folder is picked. Thus all the workflows should be moved to the github
on the root project for the workflows to actually work. Moreover it is forced by projen
that only the root construct can define the github
instance. Thus if we define a workflow in a project that is not the root, the only way for it to work is to look at the parent and set the workflow construct with a reference to the github
instance on the parent.
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.
@quesabe what I mean is that — can we move the check out of addToProject()
? So that the caller evaluates hasRustTestWorkflow
and decides if addToProject()
should be called?
dc49bf8
to
cb5aeea
Compare
cb5aeea
to
59c79da
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.
@quesabe left a question, but it's not blocking — so merging the PR. Still. have a look at the question please.
Closes PLA-289.