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
Support Windows runner for CI workflows #1275
Support Windows runner for CI workflows #1275
Changes from 63 commits
3434d63
59e6a36
2e3aae4
02f785c
ba4f774
8c6162d
58dbad1
ad6fe0f
1069183
8b216d9
864bdd7
0a95a68
235ca60
3b3f1e8
93e73c7
6284ec4
4387414
236ad95
1a6def3
0468402
2b117ae
6ad568b
4a62d9f
f7dba31
82e3a9c
a7cd2b2
00986d2
787c9ba
facdfe5
9326f3f
5edfd44
6204d6a
07a2f85
b66d256
9fcb57e
e0880e7
9ea1746
8c7111e
068373d
af37758
aed95c2
95c793f
722851f
50c48f2
fc01542
d6966e9
869cf3b
cc596f4
ca7c6dd
c619e88
9a9b362
bb352bb
b3dc358
8b798eb
a11e5ae
57b9c58
8dde509
af67535
84dd998
9954325
1836b5b
dc40463
a304c2c
5e4542b
895c627
6ae66d6
91da046
f49f3d6
6100ab8
c4f2472
3dbbe4d
9da6a34
0c95b14
677e6c1
6a6c6e2
e9fcee4
edfd2f0
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.
This looks identical to
osd-version-linux
, lets make this into a single command.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.
Did you mean
osd-version-windows
here? Also, we can combine it withosd-version-linux
to makeosd-version
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.
same as above:
osd-version-windows
instead ofosd-version-linux
?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.
Are you seeing a difference between bash/pwsh when running these commands? If it does, use
action-cond
[link] to set the platform specific shell for these commands.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.
this looks identical to the step above except shell type. you can use action conditions to specify the shell at run time
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.
See if you can use action condition to combine this step with the one above using action conditions
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.
Replace
@latest
with either a specific version tag or commit id.https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsuses
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.
Note; this applies to a couple of different places in these changes
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 for the review Peter! I have changed the firfox one to a specific version. But for the gecko I think it only has the version control of
latest
.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 see any changes in this fork, lets use
browser-actions/setup-firefox
for all platforms.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 just tested if I change it back it will be give you the error of this on Windows:
That's why I left that comment at L41.
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 see any changes in your fork, can you link the commit with the changes you needed? I'm fine using your fork and should use the same fork for both linux/windows workflows
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.
Lets reduce the duplication between the cypress and integration tests workflow. There are a couple ways this can be done.
I would prefer the first option to make a common action they reuse, that could be re-used by other teams, what do you think?
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 for the review! I have created a new action for the plugin download and script setup~
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.
Are all of the tests in
saml_auth.test.ts
failing on windows or just theTenancy persisted after Logout in SAML
test case? If its just the one test case what do you think about taking the test case out ofsaml_auth.test.ts
and putting it into another file (maybesaml_auth_tenancy.test.ts
?) and temporarily ignoring that suite while it awaits a fix?