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

feat: Add server that provides k-apps contents #2680

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

mikolajb
Copy link
Contributor

@mikolajb mikolajb commented Sep 30, 2024

What problem does this PR solve?:
Produce a docker image with http server.

Which issue(s) does this PR fix?:
https://jira.nutanix.com/browse/NCN-102822

Special notes for your reviewer:
To move things quickly, I'm using github.com/sigoden/dufs. I use it because it supports tls but I'm planning to create our own (maybe a few lines in Python - let's see what's the fastest).

Does this PR introduce a user-facing change?:


Checklist

  • If the PR adds a version bump, ensure there is no breaking change in Licensing model (or NA).
  • If a chart is changed or app configuration is significantly changed, the chart version is correctly incremented (so that apps are not automatically upgraded from a previous version of DKP).

@mikolajb mikolajb self-assigned this Sep 30, 2024
@mikolajb mikolajb requested a review from a team September 30, 2024 14:57
@github-actions github-actions bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 30, 2024
@github-actions github-actions bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 1, 2024
@github-actions github-actions bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 1, 2024
@gracedo gracedo added ready-for-review ok-to-test Signals mergebot that CI checks are ready to be kicked off labels Oct 1, 2024
Copy link
Contributor

@takirala takirala left a comment

Choose a reason for hiding this comment

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

Will this server be a thing in 2.14 (or if/when we have OCI support) ? If not, wdyt of using just a simple container image that has the necessary artifacts at a certain known path. pulling it as init container will be enough. I am not against the idea of server but this seems like an overkill for a soon to be dead code (and for something that is very short lived - bootstrap job takes less than 10m in happy path).

@mikolajb
Copy link
Contributor Author

mikolajb commented Oct 3, 2024

Will this server be a thing in 2.14 (or if/when we have OCI support) ? If not, wdyt of using just a simple container image that has the necessary artifacts at a certain known path. pulling it as init container will be enough. I am not against the idea of server but this seems like an overkill for a soon to be dead code (and for something that is very short lived - bootstrap job takes less than 10m in happy path).

Yes, I get it, the choice is between having a server and relying on a git operator way of initializing the repository (that won't change when we move to OCI) or having an init container and creating a custom way of initializing the repository that will go away when we move to the OCI. I would favor the former.

In other words, this web server fits in the APIs and git operator not the other way around.

Base automatically changed from mikolajb/unify-release-code to main October 3, 2024 14:31
@takirala
Copy link
Contributor

takirala commented Oct 3, 2024

So in the current way, we would have to:

  • refactor kapps tooling to publish oci image in 2.14
  • refactor bootstrap job to consume oci repo (instead of fetching from http server)

But, if we can publish a docker image (that is very close a future oci image, basically a simple image that has some file system) then we can keep the future refactoring minimal by

  • refactoring kapps by pushing the oci image instead of the plain docker image (this refactor ll be much more minimal compared to whats being done in this PR)
  • refactoring bootstrap job to get the content using https://kubernetes.io/docs/tasks/configure-pod-container/image-volumes/ in 2.14 or getting the same contents from .status.artifact.url of OCIRepository which will again be a signifcantly smaller refactor compared to moving away from hitting an http endpoint.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11167419015

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 51.557%

Totals Coverage Status
Change from base Build 11163988221: 0.0%
Covered Lines: 149
Relevant Lines: 289

💛 - Coveralls

@mikolajb
Copy link
Contributor Author

mikolajb commented Oct 4, 2024

So in the current way, we would have to:

  • refactor kapps tooling to publish oci image in 2.14
  • refactor bootstrap job to consume oci repo (instead of fetching from http server)

We discussed it on Slack but for someone reading it: both are already happening actually. The oci artifact is already being published since 2.12. Git operator is being able to consume OCI artifact when initializing a repository.

In other words: this method is reducing the future refactoring to the absolute minimum.

@mikolajb mikolajb merged commit e73f0e4 into main Oct 7, 2024
16 checks passed
@mikolajb mikolajb deleted the mikolajb/k-apps-server branch October 7, 2024 22:40
ArvinderPal09 added a commit that referenced this pull request Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Signals mergebot that CI checks are ready to be kicked off ready-for-review size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants