-
Notifications
You must be signed in to change notification settings - Fork 182
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
Improvements to Container CD #385
Conversation
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.
All I see is the minor doc change. other than that, seems like an elegant architecture improvement.
d1f3947
to
e02e651
Compare
How does this new structure combined with current compose and k8s functionality test? Our test machine are not able to access an internal docker registry, so we have 2 local registry for CI test. We need to sync about the design. |
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 @tylertitsworth for the PR.
My change requests are mostly minor except for introducing dependency on ai-containers/.github@main
which needs to be reviewed by the community.
.github/workflows/container-ci.yml
Outdated
# Get diff array filtered by specific filetypes | ||
DIFF=$(git diff --diff-filter=d \ | ||
--name-only ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} \ | ||
-- '*/*Dockerfile' '*.py' '*.yaml' '*.yml' '*.sh' '*/*requirements.txt' '*.json' '*.ts' '*.js' | \ |
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.
there may be a few more file types and extensions:
*.dockerfile, *Dockerfile*, *.cjs, *.svelte and etc
@chensuyue the current compose structure could be merged with this design. It's all about having a consistent file system and making sure your dependencies are in subfolders so you can accurately track changes to individual containers. Regarding the local CI, we can change the runner labels and just make sure to use the correct secrets. Regarding k8s, I believe we previously spoke about how to best test helm charts. We have a composite action for this. However I see a lot of manifests so it might be better to utilize the baremetal test runner mode to call your existing test scripts to validate those manifests. If I were smarter I would've made a branch rather than a fork so I could actually test these things, it's hard to integrate GitHub CI without maintainer access. |
Thanks @ashahba, it's common practice to re-use GitHub actions from other locations. I hope that the community will accept another intel repository as a trusted and maintained source similar to how they trust GitHub, docker, helm, pre-commit.ci, and other open source tools already in use. If the fear of a dependency failing is important. These actions can be pinned at a specific SHA to avoid failures like I did with many other actions introduced by this workflow. |
Signed-off-by: tylertitsworth <[email protected]> add chatqna support Signed-off-by: tylertitsworth <[email protected]> update tests and repo path Signed-off-by: tylertitsworth <[email protected]> add actions.json file for ci configuration Signed-off-by: tylertitsworth <[email protected]> add codegen Signed-off-by: tylertitsworth <[email protected]> add weekly test Signed-off-by: tylertitsworth <[email protected]> remove ref Signed-off-by: tylertitsworth <[email protected]> change runner label Signed-off-by: tylertitsworth <[email protected]> do the rest of them Signed-off-by: tylertitsworth <[email protected]> copy workflow call into repo due to gha bug #2877542 Signed-off-by: tylertitsworth <[email protected]> configure no start Signed-off-by: tylertitsworth <[email protected]> patch scan and test wfs Signed-off-by: tylertitsworth <[email protected]> change to more descriptive name for reusable workflow Signed-off-by: tylertitsworth <[email protected]> additional bugfix Signed-off-by: tylertitsworth <[email protected]> update scan workflow Signed-off-by: tylertitsworth <[email protected]> see help menus Signed-off-by: tylertitsworth <[email protected]> add missing env Signed-off-by: tylertitsworth <[email protected]> use downcase method Signed-off-by: tylertitsworth <[email protected]> remove test Signed-off-by: tylertitsworth <[email protected]> use valid group dir for rest of scan workflow Signed-off-by: tylertitsworth <[email protected]> return import step Signed-off-by: tylertitsworth <[email protected]> add and validate smoke tests Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 | ||
- uses: docker/login-action@0d4c9c5ea7693da7b068278f7b52bda2a190a446 # v3.2.0 | ||
with: | ||
registry: ${{ secrets.REGISTRY }} |
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.
How to use a common private registry on IDC machine? Now we have 2 local registry separately for xeon and gaudi cluster.
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.
If you can use something like harbor that can be accessed by both clusters or a cloud registry that would be ideal.
If your runners have pre-authenticated access to these local registries, then we could remove the login commands/secrets and use environment variables that are present in the runner environment.
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: Tyler Titsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Signed-off-by: tylertitsworth <[email protected]>
Description
Update CD to follow DevOps best practices.
How it works
Given an example(s) (AudioQnA), this workflow creates jobs that perform container CI on that example, and allow other event-based workflows to curate a matrix of these examples.
These are the inputs for the composites:
Build
Scan
This is the inputs for the reusable workflow:
Container CI
These are the Events that this PR supports:
Changes
harden-runner
status-check
flag inpull_request
events that verifies if any job inexample-ci.yaml
failed.Issues
n/a
Type of change
List the type of change like below. Please delete options that are not relevant.
Dependencies
n/a
Tests
I don't have the runners required to do the validation portion, however when previously testing with
ubuntu-latest
I was able to launch the test script from the correct directory on that runner before it ran out of space.https://github.com/tylertitsworth/GenAIExamples/actions/runs/9896085740?pr=3