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

packaging: Integrate github workflows to automate the air-gapping process of wave university #2181

Merged
merged 28 commits into from
Feb 8, 2024

Conversation

sulhicader
Copy link
Contributor

The PR fulfills these requirements: (check all the apply)

  • It's submitted to the main branch.
  • When resolving a specific issue, it's referenced in the PR's title (e.g. feat: Add a button #xxx, where "xxx" is the issue number).
  • When resolving a specific issue, the PR description includes Closes #xxx, where "xxx" is the issue number.
  • If changes were made to ui folder, unit tests (make test) still pass.
  • New/updated tests are included

This closes issue #2163

@sulhicader sulhicader self-assigned this Nov 2, 2023
@mturoci
Copy link
Collaborator

mturoci commented Nov 20, 2023

Thanks for the PR @sulhicader!

What's the reason for splitting into multiple files/workflows? It seems like an unnecessary abstraction that introduces plenty of duplication which will be hard to maintain for a few relatively trivial tasks - build and publish docker/helm.

Can we put it into existing workflow files instead? Also, it seems like you are only doing this for Wave University app so there is no need to make it overly parametrized, and hardcoding for simplicity should be fine for now.

@sulhicader
Copy link
Contributor Author

@mturoci Thanks you for the inputs.

.github/workflows/wave-bundle-docker-build-publish.yaml, .github/workflows/wave-bundle-helm-release.yaml workflows will going to be common for all the apps going to pusblish from wave repo ( for now university and tour ). Also I mainly using the workflows from https://github.com/h2oai/wave-template/tree/main/.github/workflows template repo and in future if any additions comes to it, it will be easier to reflect those here. Here new workflows are triggered from .github/workflows/publish-university.yml which is existing workflow. As you suggest We can reduce the workflow files but putting into one file, but it also deviate from the standard template. WDYT?

@mturoci
Copy link
Collaborator

mturoci commented Nov 23, 2023

The template is designed to build an arbitrary app for scratch, not to be integrated into existing workflows like here so it doesn't make sense to introduce it since all the apps have their builds already defined.

  • The builds only need to be adjusted to use h2o cli instead of plain zip.
  • Add steps to push to ECR / Helm as needed.

Correct me if I missed something in ^^.

Also, if anything changes, I can guarantee you (or anyone) will be able to resolve it much faster in 2 files with proper context than juggling across 6 files.

@sulhicader
Copy link
Contributor Author

Sure @mturoci, I am working on this.

@johnygomez johnygomez removed their request for review December 7, 2023 09:57
@sulhicader
Copy link
Contributor Author

Hi @mturoci, I addressed the review comments and took the functionalities into one script. Please feel free to add comments if any changes needed.

@mturoci mturoci merged commit 31c8957 into main Feb 8, 2024
2 checks passed
@mturoci mturoci deleted the feat/issue-2163 branch February 8, 2024 08:23
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