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

[TKET-1847] Update workflows for tket v2 #265

Merged
merged 23 commits into from
Mar 3, 2022
Merged

Conversation

cqc-melf
Copy link
Contributor

@cqc-melf cqc-melf commented Mar 1, 2022

adding changes to the workflows for routing v2

@cqc-melf cqc-melf requested a review from cqc-alec March 1, 2022 17:30
@cqc-melf
Copy link
Contributor Author

cqc-melf commented Mar 1, 2022

Happy to have a discussion about my changes, please message me in case of any questions.

.github/workflows/build_and_test_v2.yml Outdated Show resolved Hide resolved
.github/workflows/create_pr_v2.yml Outdated Show resolved Hide resolved
.github/workflows/docs_v2.yml Outdated Show resolved Hide resolved
.github/workflows/docs_v2.yml Outdated Show resolved Hide resolved
.github/workflows/release_v2.yml Outdated Show resolved Hide resolved
@cqc-melf cqc-melf requested a review from cqc-alec March 2, 2022 15:09
@cqc-melf
Copy link
Contributor Author

cqc-melf commented Mar 2, 2022

I have added the changes from #269 to the new workflows in the commit 0408239

@cqc-alec
Copy link
Collaborator

cqc-alec commented Mar 2, 2022

Should we add the valgrind check for PRs to develop2 as well?

@cqc-melf
Copy link
Contributor Author

cqc-melf commented Mar 2, 2022

Should we add the valgrind check for PRs to develop2 as well?

Good point, from my understanding this could be done by adding develop2 to the workflow?

I have added that in @cqc-melf
add valgrind for develop2
6215eaf

@cqc-alec
Copy link
Collaborator

cqc-alec commented Mar 2, 2022

I think you made a mistake with the merge? This commit looks unintended.
The valgrind fix is good for now, but I imagine when the branches diverge they will want different workflows.

@cqc-melf
Copy link
Contributor Author

cqc-melf commented Mar 2, 2022

I think you made a mistake with the merge? This commit looks unintended. The valgrind fix is good for now, but I imagine when the branches diverge they will want different workflows.

Oh yes, thank you. Do you want me to split the valgrind workflow now?

@cqc-alec
Copy link
Collaborator

cqc-alec commented Mar 2, 2022

Oh yes, thank you. Do you want me to split the valgrind workflow now?

No it's fine.

@cqc-alec
Copy link
Collaborator

cqc-alec commented Mar 2, 2022

linuxbuildwheel has become non-executable.

Copy link
Collaborator

@cqc-alec cqc-alec left a comment

Choose a reason for hiding this comment

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

Please make linuxbuildwheel executable again, otherwise LGTM.

Comment on lines 10 to 12
schedule:
# 03:00 every Saturday morning
- cron: '0 3 * * 6'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, we don't want this. This will just run it on the default branch (develop) -- which is already happening. Is there a way to run the schedule on a non-default branch?

Comment on lines 7 to 9
schedule:
# 03:00 every Monday morning
- cron: '0 3 * * 1'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly here. This means I think that we do need a separate workflow for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like that is possible, I will try to do that.

@cqc-melf
Copy link
Contributor Author

cqc-melf commented Mar 2, 2022

@cqc-alec I have found a solution which require to duplicate all the workflows to make it work. I think that is fine because we will split that up soon anyway?

@cqc-melf cqc-melf requested a review from cqc-alec March 2, 2022 17:35
- cron: '0 3 * * 1'

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does github.workflow evaluate to the name of the yml file (valgrind_v2_nightly) or the name of the workflow (valgrind check nightly)? If the latter it needs to be changed so that it is unique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

valgrind check nightly already was unique, but I have changed that for clarity.

@@ -3,6 +3,7 @@ on:
pull_request:
branches:
- develop
- develop2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having add the new one this should not be added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to push the commit, sorry. It is up now in @cqc-melf
remove develop2 from valgrind check
84ca79c

- uses: actions/checkout@v2
with:
fetch-depth: '0'
ref: develop2 - run: git fetch --depth=1 origin +refs/tags/*:refs/tags/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

repaired

@@ -0,0 +1,436 @@
name: Build and test v2 nightly
Copy link
Collaborator

@cqc-alec cqc-alec Mar 3, 2022

Choose a reason for hiding this comment

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

Do we really need a new file for this? Why can't we just put the ref: develop2 into the original file to make sure it runs on the right branch?
Same goes for the valgrind workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding it is necessary to do it like this. If we change the branch in the PR workflow every check run on a PR would only evaluate the current state of develop2? Or have I misunderstood what you mean?

The only option I see is to integrate the valgrind_v2 into the valgrind check, but that probably need to be changes in the very near future anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yeah you're right.
It's really painful having so much boilerplate repeated in different workflows. We should look at using https://docs.github.com/en/actions/using-workflows/reusing-workflows .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think it is a bit annoying, too. Should I do that now or is that a task for later?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I think it is a bit annoying, too. Should I do that now or is that a task for later?

I think we can defer it but should prioritize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make a task for it.

@cqc-melf cqc-melf requested a review from cqc-alec March 3, 2022 11:07
@@ -3,6 +3,7 @@ on:
pull_request:
branches:
- develop
- develop2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still there?

@cqc-melf cqc-melf merged commit 6783400 into develop Mar 3, 2022
@cqc-melf cqc-melf deleted the update-workflows-v2 branch March 3, 2022 12:32
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.

2 participants