Skip to content

xdna-driver CI #24

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

Closed
wants to merge 2 commits into from
Closed

xdna-driver CI #24

wants to merge 2 commits into from

Conversation

amd-akshatah
Copy link

CI source is not enabled by default. This will be enabled after the secrets are added , runners are added and internal repo is tested.

CI to run main flow
CI for PR authentication
steps:
- name: Checkout private repository
if: ${{ github.event_name == 'pull_request_target' }}
uses: actions/checkout@v3
Copy link
Contributor

Choose a reason for hiding this comment

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

To update to v4 everywhere I guess.

gitExternalRepo: ${{ secrets.GIT_EXTERNAL_REPO }}

upload:
if: ${{ always() }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it interesting to upload in the case the workflow has been canceled?

Copy link
Author

Choose a reason for hiding this comment

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

upload basically uploads the log file and internal users always want to see the log irrespective of whether the workflow is cancelled. The reason why we are uploading the log is because we are hiding the console output of the flow and routing it to a log file for internal use only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. You could add this as a comment to enlighten the reader.

jobs:
check:
runs-on: [ Ubuntu-22.04 ]
if: contains('amd-akshatah, kingtamamd, maxzhen, raphaelbamd, sonals, shirishjo ', format('{0},', github.actor)) && github.event.comment.body == '/build' && github.event.issue.pull_request
Copy link
Contributor

@keryell keryell Mar 6, 2024

Choose a reason for hiding this comment

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

Since this is security-related and a bit convoluted, is this safe? It is unclear.
For example, if I have a user sonal or hisonals, does it work?
Relying on string conversion looks scary to me.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, Let me check if I can add a group instead of string with a change in this logic.

@keryell
Copy link
Contributor

keryell commented Mar 6, 2024

I am curious about which kernel is actually used to run the CI.
It would be nice to have some tests against real kernels people will use in production. Cf. #3 for example.

@maxzhen
Copy link
Collaborator

maxzhen commented Mar 6, 2024

I am curious about which kernel is actually used to run the CI. It would be nice to have some tests against real kernels people will use in production. Cf. #3 for example.

Production testing is separate. CI is just to sanity test the PR. As for the kernel, it will be close to latest released Linux kernel.

@amd-akshatah amd-akshatah deleted the CI branch March 7, 2024 01:00
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