-
Notifications
You must be signed in to change notification settings - Fork 65
feat: Migrate vwa workflows clean #625
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
base: notebooks-v1
Are you sure you want to change the base?
feat: Migrate vwa workflows clean #625
Conversation
* chore: Adding ADOPTERS.md file Signed-off-by: Francisco Javier Arceo <[email protected]> * fix typo from copy/paste Signed-off-by: Francisco Javier Arceo <[email protected]> * including openshift ai Signed-off-by: Francisco Javier Arceo <[email protected]> * Update ADOPTERS.md Co-authored-by: Eder Ignatowicz <[email protected]> Signed-off-by: Francisco Arceo <[email protected]> * Update ADOPTERS.md Signed-off-by: Francisco Arceo <[email protected]> --------- Signed-off-by: Francisco Javier Arceo <[email protected]> Signed-off-by: Francisco Arceo <[email protected]> Co-authored-by: Eder Ignatowicz <[email protected]>
* chore: create issue templates for project planning related: kubeflow#327 This commit creates 3 new GitHub issue templates to help with Notebooks 2.0 project planning. As recently agreed upon in community meetings - we want to break down work into 3 hierarchical categories: - Epics - Features - Tasks The templates defined in this pull request mimic the format @ederign used when initially defining work for our first "sprint" of work. :warning: Please note, however, I have introduced the term `Task` to capture the most granular issue type. Happy to change the word to something else - but wanted a `[...]` prefix to identify these issues similar to `[EPIC]` and `[FEATURE]` These issue templates would not be intended to be used by "random" community members - rather they are designed to aid Epic Owners in defining work. For now, I have just added a simple "disclaimer" in the issue template summary to discourage use. In the future we could discuss extending `internal-acls` and leveraging GitHub Actions to ensure these "planning" templates are only used by designated community members. The `Epic` and `Feature` templates **DO NOT** have a section to track child items included in the issue description. The desire to merge kubeflow#325 to leverage GitHub sub-issues to more naturally track these relationships and avoid having to "ping" issue authors to update the description to track children. The `config.yml` `issue_templates` section is used to define a deterministic order within the GitHub Template Chooser modal. As we can control the order through this file along - I also removed the leading numerical prefixes from existing yml template files as they serve no purpose and at worst could actively confuse. Signed-off-by: Andy Stoneberg <[email protected]> * mathew: update epic label Signed-off-by: Mathew Wicks <[email protected]> * Update .github/ISSUE_TEMPLATE/planning_task.yml Signed-off-by: Mathew Wicks <[email protected]> * mathew: update feature label Signed-off-by: Mathew Wicks <[email protected]> * mathew: remove emoji from planning names Signed-off-by: Mathew Wicks <[email protected]> --------- Signed-off-by: Andy Stoneberg <[email protected]> Signed-off-by: Mathew Wicks <[email protected]> Co-authored-by: Mathew Wicks <[email protected]>
Signed-off-by: Andrey Velichkevich <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
@henschwartz - I would encourage you to reach out to @liavweiss and/or @yehudit1987 to get a better sense of expected changes for this work.
I'm not yet going to do a deep review on the code - as they should be able to help out to get this is better shape for an initial review.
In general - the following areas need improved:
- ensure
kubeflow/notebooks
(notkubeflow/kubeflow
listed as part of the container registry - ensure the
branches:
config lists the following:main
(notmaster
)notebooks-v1
'v*-branch'
- ensure files added to this PR have valid content
- I'm seeing numerous "empty files" which I am guessing was unintentional
Getting clean GHA check results for these workloads on your fork would likely highlight these shortcomings - so I encourage you to look into that. I am happy to help if you want to talk about how to accomplish that.
1d54153
to
c363d91
Compare
c363d91
to
c0cf955
Compare
e5437dd
to
2ac78c1
Compare
33f4706
to
515fd68
Compare
Signed-off-by: Hen Schwartz (EXT-Nokia) <[email protected]>
515fd68
to
93bccd1
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.
Thank you for your continued work on this PR... getting closer to having something in desired shape to get this merged! 💯
In addition to the comments I have raised on specific lines throughout the PR - please rebase your changes off the latest notebooks-v1
branch so the testing/gh-actions
files can be "picked up" from the notebooks-v1
branch now that they are available.
branches: | ||
- main | ||
- notebooks-v1 |
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.
Need to also include the "release branch" to trigger this workflow - just like its defined in the original kubeflow/kubeflow
source of truth:
branches: | |
- main | |
- notebooks-v1 | |
branches: | |
- main | |
- notebooks-v1 | |
- v*-branch |
frontend-format-lint-check: | ||
name: Check code format and lint | ||
runs-on: ubuntu-22.04 | ||
steps: | ||
- name: Check out code | ||
uses: actions/checkout@v4 | ||
|
||
- name: Setup Node | ||
uses: actions/setup-node@v4 | ||
with: | ||
node-version: 16 | ||
|
||
- name: Check frontend code formatting | ||
run: | | ||
cd components/crud-web-apps/volumes/frontend | ||
npm i | ||
npm run format:check | ||
|
||
- name: Check frontend code linting | ||
run: | | ||
cd components/crud-web-apps/volumes/frontend | ||
npm i | ||
npm run lint-check |
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.
Please see this comment I made on a PR from @yehudit1987 that talks about how we can streamline this particular job (given its isolation/simplicity)
- name: Install Firefox dependencies | ||
run: | | ||
sudo apt-get update | ||
# Install Firefox from snap (more reliable for CI) | ||
sudo snap install firefox --classic | ||
# Install xvfb for headless mode | ||
sudo apt-get install -y xvfb | ||
# Create multiple symlinks to ensure Firefox is found | ||
sudo ln -sf /snap/bin/firefox /usr/local/bin/firefox | ||
sudo ln -sf /snap/bin/firefox /usr/bin/firefox | ||
# Add snap bin to PATH for this session | ||
echo 'export PATH="/snap/bin:$PATH"' >> ~/.bashrc | ||
export PATH="/snap/bin:$PATH" | ||
# Verify Firefox installation and location | ||
which firefox | ||
firefox --version | ||
# Set up display for headless mode | ||
export DISPLAY=:99 | ||
Xvfb :99 -screen 0 1024x768x24 > /dev/null 2>&1 & | ||
# Wait a moment for Xvfb to start | ||
sleep 2 |
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.
According to a Slack DM I got from @yehudit1987 - it seems Cypress/Firefox issues can be resolved by upgrading Cypress to 14.1.0
.
Can you give that a try to see if it avoid having to do this complicated configuration step?
Additionally - these types of (legitimate) changes to the codebase should be called out in the commit message for posterity sake!
branches: | ||
- main | ||
- notebooks-v1 |
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.
Need to also include the "release branch" to trigger this workflow - just like its defined in the original kubeflow/kubeflow
source of truth:
branches: | |
- main | |
- notebooks-v1 | |
branches: | |
- main | |
- notebooks-v1 | |
- v*-branch |
branches: | ||
- main | ||
- notebooks-v1 |
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.
Need to also include the "release branch" to trigger this workflow - just like its defined in the original kubeflow/kubeflow
source of truth:
branches: | |
- main | |
- notebooks-v1 | |
branches: | |
- main | |
- notebooks-v1 | |
- v*-branch |
- name: Build multi-arch images | ||
uses: docker/build-push-action@v5 | ||
with: | ||
context: components/crud-web-apps | ||
file: components/crud-web-apps/volumes/Dockerfile | ||
platforms: ${{ env.PLATFORMS }} | ||
push: false | ||
load: false | ||
tags: | | ||
${{ env.IMG }}:${{ github.sha }} | ||
${{ env.IMG }}:latest | ||
cache-from: type=gha | ||
cache-to: type=gha,mode=max |
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.
Please see related comments I have made on @yehudit1987 's PR that includes a similar build-push-action
step:
- feat: Migrate JWA test-related workflows from kubeflow/kubeflow to notebooks-v1 branch #587 #599 (comment)
- feat: Migrate JWA test-related workflows from kubeflow/kubeflow to notebooks-v1 branch #587 #599 (comment)
- feat: Migrate JWA test-related workflows from kubeflow/kubeflow to notebooks-v1 branch #587 #599 (comment)
migrate VWA test workflows from kubeflow/kubeflow to notebooks-v1
[TASK] Migrate VWA test-related workflows from kubeflow/kubeflow to notebooks-v1 branch #586