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

Empower vendir to make an informed decision on what the overall semver of a directory is #165

Open
voor opened this issue Jun 16, 2022 · 14 comments
Assignees
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request

Comments

@voor
Copy link

voor commented Jun 16, 2022

Describe the problem/challenge you have
After I sync things with vendir and create my informed view of a folder I need to make a decision later on about what overall semantically appropriate version this folder is. Today I do that with some yq glue:

  if [ -f "${PWD}/version-override.yml" ]; then
    APP_VERSION=$(yq eval '.. | select(has("tag")).tag' version-override.yml)
  else
      APP_VERSION=$(yq eval '.. | select(has("tag")).tag, .. | select(has("tags")).tags[0]' vendir.yml vendir.lock.yml | head -n1)
  fi

This is later surfaced with the Carvel Packaging API, since it requires semver to properly function.

Describe the solution you'd like
I later on need to populate a Package CR and it needs a semver passed into it. If vendir had some sort of command, like vendir lock-config-tag -d dir/sub-dir that could spit out something properly formatted for the Packaging API, I would be happy.

Anything else you would like to add:
I'm not sure what to do as a failover, or what happens when vendir can't ascertain what the versioning is if nothing in the folder itself is semver.


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@voor voor added carvel-triage This issue has not yet been reviewed for validity enhancement This issue is a feature request labels Jun 16, 2022
@github-actions
Copy link

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

@github-actions github-actions bot added the stale This issue has had no activity for a while and will be closed soon label Jul 27, 2022
@voor
Copy link
Author

voor commented Jul 27, 2022

/remove-stale

@joaopapereira
Copy link
Member

Not sure I fully understand the use case. Let me try to paraphrase it to see if I got your problem.
You are using vendir to sync a folder.
In some situations, you have a file called version-override.yml that contains a field called tags that will contain the semver version of the current folder.
In other situations, you will have to find the tags inside the vendir.lock

You use this value when you are generating the PackageCR so that you can populate the version field of it.

Is this what you are doing now?

What you are asking is for vendir to have a command that:

  1. Generates the PackageCR for you?
  2. To extract the version from the vendir.lock and provide it to you so that you do not have to use the yq glue?

For the overriding option, what is your suggestion? That the override file can be provided as an argument to this new command?

@joaopapereira joaopapereira removed the stale This issue has had no activity for a while and will be closed soon label Jul 27, 2022
@voor
Copy link
Author

voor commented Jul 28, 2022

This is more heavily involved in 2 in your above commands. The Package CR is going to be created elsewhere by the kctrl pkg release stuff. Except for kctrl to work it needs a guarantee that it's getting a semver acceptable value (otherwise it'll fail). The connecting point here is that vendir can effectively provide that guarantee since it knows the contents of what will ultimately go into this package.

I envision something like vendir lock-config-tag -d dir/sub-dir such that you can combine this with kctrl:

kctrl pkg release --version $(vendir lock-config-tag -d dir/sub-dir --default-version v0.0.1-rc.1)

Where vendir will look at creating a semver based on the information it has on-hand (like a git tag, an image tag, a helm version) so that this entire process can be more aligned to the upstream resources version.

@voor
Copy link
Author

voor commented Jul 28, 2022

Massive bonus points and make this a significantly more pleasurable experience if the default-version could be declared somewhere inside the vendir spec file so it doesn't need to be a cli argument.

@joaopapereira
Copy link
Member

I think I understand better now what you are asking for.

What would you expect vendir to return if the lock file contains multiple folders with tags? Something like this:

apiVersion: vendir.k14s.io/v1alpha1
kind: LockConfig

directories:
- path: config/_ytt_lib
  contents:
  - path: github.com/cloudfoundry/cf-k8s-networking
    git:
      sha: 2b009b61fa8afb330a4302c694ee61b11104c54c
      commitTitle: 'feat: add /metrics prometheus scrapable endpoint...'
      tags:
      - "4.0.0"
  - path: some/other/path
    image:
      url: index.docker.io/dkalinin/consul-helm@sha256:d1cdbd46561a144332f0744302d45f27583fc0d75002cba473d840f46630c9f7
      tag: "some-tag"

@voor
Copy link
Author

voor commented Jul 28, 2022

some-tag is not valid semver, so it would be discarded, and you'd be left with just 4.0.0.

I imagine some sort of iterative logic for a "MVP" would suffice -- go through the lock file's contents of tags and tag and find the first semver valid one, or failover to the default if nothing is found.

@joaopapereira
Copy link
Member

joaopapereira commented Aug 26, 2022

Sorry I dropped the ball on this issue.

The iteration and retrieving the first semver seems like a very error prune way to handle this, because it forces the user who created the Config to order the directories in a specific way, it almost feels like there should be a root level parameter that you can fill up the version on, something like:

apiVersion: vendir.k14s.io/v1alpha1
kind: LockConfig

version: 3.9.0
directories:
- path: config/_ytt_lib
  contents:
  - path: github.com/cloudfoundry/cf-k8s-networking
    git:
      sha: 2b009b61fa8afb330a4302c694ee61b11104c54c
      commitTitle: 'feat: add /metrics prometheus scrapable endpoint...'
      tags:
      - "4.0.0"
  - path: some/other/path
    image:
      url: index.docker.io/dkalinin/consul-helm@sha256:d1cdbd46561a144332f0744302d45f27583fc0d75002cba473d840f46630c9f7
      tag: "1.0.0"

If we just rely on a first found semver that in the example above(ignoring the field version I added) if we changed the tag of the image to 1.0.1 and you would not find a difference in version between the 2 LockConfigs because they would give you 4.0.0

@joaopapereira
Copy link
Member

let me know if my suggestion make sense

@voor
Copy link
Author

voor commented Sep 1, 2022

Could you templatize the root version to get it's entry from another one?

@kumaritanushree kumaritanushree self-assigned this Sep 28, 2022
@joaopapereira
Copy link
Member

I think we could definitly try and do some sort of jsonpath to have some templating of that version. Something like:

apiVersion: vendir.k14s.io/v1alpha1
kind: LockConfig

version: #@ directives[0].contents[0].git.tags[0]
directories:
- path: config/_ytt_lib
  contents:
  - path: github.com/cloudfoundry/cf-k8s-networking
    git:
      sha: 2b009b61fa8afb330a4302c694ee61b11104c54c
      commitTitle: 'feat: add /metrics prometheus scrapable endpoint...'
      tags:
      - "4.0.0"
  - path: some/other/path
    image:
      url: index.docker.io/dkalinin/consul-helm@sha256:d1cdbd46561a144332f0744302d45f27583fc0d75002cba473d840f46630c9f7
      tag: "1.0.0"

This might increase the scope of this story a little bit. @kumaritanushree since you are going to pick up this work, please take a look at this templatization idea, if you feel like it is going to be a big chunk of work, let us know and we can split it into a different story

@joaopapereira joaopapereira added carvel-accepted This issue should be considered for future work and that the triage process has been completed and removed carvel-triage This issue has not yet been reviewed for validity labels Sep 28, 2022
@kumaritanushree
Copy link
Contributor

Sure @joaopapereira , will have a look into this in detail and get back to you soon.

@neil-hickey
Copy link
Contributor

hey @kumaritanushree / @joaopapereira did this work get split off into another issue?

@kumaritanushree
Copy link
Contributor

@neil-hickey Because of other high priority tasks did not get time to look into this. Based on the priority of tasks will try to look back into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request
Projects
Status: Unprioritized
Development

No branches or pull requests

4 participants