-
Notifications
You must be signed in to change notification settings - Fork 0
Helm backend #9
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
Helm backend #9
Conversation
- Add TARGETARCH buildarg to migration job Dockerfile for local testing - Fix workload migration to use correct appType parameter and sequence starting value - Omit id from Workload and Helm configs to avoid confusion - List all missing env vars - Fix incorrect image registry URL - Ignore not found error when deleting a namespace - Fix various API handlers - Fix async error handling during app validation - Do not await deployment process after validation and creating a Deployment - Update git version
- Write types for the form states of configs of different sources - Add helper functions for form state and config generation - Move source or app type-specific code into separate components - Separate DiffInput into DiffInput and DiffSelect for speed
Tested on an x86_64 system (TARGETARCH=amd64). Works with `docker build` and from Tilt. Should work on 64-bit ARM systems; won't work on 32-bit ARM since Tini's binary naming convention doesn't line up. Source: https://github.com/BretFisher/multi-platform-docker-build
…flictError handler
Not sure if this changes semantics but it matches the convention of the other functions in the file. Maybe try/catch behavior is different?
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.
Phew! This PR is gigantic. I've looked over everything but I haven't tested it myself yet.
I made some small changes that shouldn't change any behavior. Here is that diff if you'd like to review: https://github.com/PurdueRCAC/AnvilOps/compare/9eebc7979352eaa53f3d0ee9ef962cd7f0daa93f..ec207db68386c72474b82623e8cb45446bcf2883.
General notes:
- I really like the idea of data-driven Helm templates that allow us to create forms with JSON and validate with JSON schemas. Great idea.
- The refactoring generally looks good and I like the way you're doing dependency injection in
src/service/helper/*. - Thanks for splitting off the frontend form stuff into separate files based on configuration area. That makes it a lot nicer to work with.
- Administrators should have the ability to disable Helm deployments via environment variable. Maybe even Git and Image deployments too for completeness.
- Of course, documentation needs to be updated to explain how administrators configure a Helm OCI repo & how users use the charts.
- On the subject of documentation, I think the codebase is separated enough that we should document the directory structure somewhere.
Everything else is in individual review comments.
| if (!isWorkloadConfig(app.config)) { | ||
| return ( | ||
| <div className="text-center py-8"> | ||
| <p>Configuration editing is not available for Helm-based apps.</p> |
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 assume this is temporary since users need a place to edit the Helm chart values, but if not, the whole tab should be hidden just like what I do for the Files and Logs tabs when they don't have any content.
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.
Yes, this is temporary
- Move @relation to WorkloadConfig and HelmConfig for effective cascade deletion - Remove REGISTRY_API_URL environment variable - Add create and update types to prepareMetadataForApps - Return a deployment title in getDeployment - Return non-applicable fields as null from getDeployment - Cache the result of listCharts - Create or update helm apps using a Job - Hide non-applicable tabs and fields for helm apps
Otherwise, the job fails because the service user doesn't have permission to read inside a nonexistent namespace. This also adds the Rancher annotations.
Move hardcoded securityContext and resources to values.yaml - Including the same key twice(once hardcoded and once conditionally from values) resulted in yaml unmarshal errors
Changes
/chart
anvilops-valuesis some JSON that could be used for generating form fields for the chart on the frontend./openapi
/backend
/prisma
/db
/lib
helmto retrieve and upgrade charts./service
/frontend
To-do