-
-
Notifications
You must be signed in to change notification settings - Fork 47
dynamic path/tag variables #372
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: main
Are you sure you want to change the base?
Conversation
ebbb5d4
to
e4dcfdf
Compare
@codablock pls review/comment so I can finish it, thx |
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.
Added a few comments. Sorry for this taking so long.
pkg/deployment/deployment_item.go
Outdated
} | ||
div.RelPath = pe | ||
div.RevPath = pr | ||
div.RelRoot = strings.Repeat("../", len(strings.Split(di.RelToSourceItemDir, "/"))) |
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.
what does this try to achieve?
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.
it creates ../../../
,, assume as prefix to get project root.
what ever you are nested, to load any shared component from project root is as easy as
so to load sth. from compose
dir - with shared components (that are re-used many times in various app deployments)
/compose/_dockerconfig
/targets
.kluctl.yml
deployment.yaml
Example:
targets/baseLayer/etcd/kustomization.yml
4:- {{ this.relRoot }}/compose/_dockerconfig
5:- {{ this.relRoot }}/compose/_wingmansigner
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.
Simply using /compose/_dockerconfig
does not work? If I remember correct, it was at least supposed to work...if not, it's maybe a bug.
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.
For kustomize itself it would certainly not work if the structure is like below
├── compose
│ ├── dockerconfig
│
├── cluster
│ ├── deployment.yml
│ ├── argo
│ │ ├── kustomization.yml
├── deployment.yml
├── .kluctl.yml
✗ Building kustomize objects
✗ 1 error occurred:
* building kustomize objects for /Users/xxxx/Workspace/sre-staging-model/deployment/kube/cluster/argo failed. accumulating components: loader.New "new root '/compose/dockerconfig' cannot be absolute"
to make it working, you would have to on temp. build dir update paths of bases:, resources:, components:
some "magic" could be done here: https://github.com/kluctl/kluctl/blob/main/pkg/deployment/deployment_item.go#L443
Non-related topic
in this discussion we would even evaluate whether we can copy to "build dir" the linked resources. I can explain in detail but if Kustomize.yml under target refers a file (in bases: or components:) out of its subdirectories (above paths) then it needs to be copied to .build dir either.
I was using this, but it has many unexpected consequences.
deployments:
# WORKAROUND to append kustomize resources/dependencies to build dir
#- service directory is rendered once using global params
{% for s in params.keys(). %}
- path: ../service/{{ s }}
onlyRender: true
vars:
- file: ../service/{{ s }}/context.yml
ignoreMissing: true
{% endfor %}
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.
Ok I see, even if kluctl templating would be able to load from /
(pointing to the project root), kustomize would fail as it simply does not allow absolute pathes.
I'll think about this and maybe create one or two commits in regard to naming of these vars.
This will be important for you as well: #416 For you this means that your structs need to be updated with json tags. |
pkg/deployment/deployment_project.go
Outdated
if item.Path != nil { | ||
// Validate or fix item.Path, as k8s labels has constrains | ||
// regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?') | ||
if *item.Path == "." { |
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.
This change make this possible ((path: .) & highlight that for example _stage0
as invalid - we can consider some validation later, possibly even later on di.Tags
deployments:
- path: . #<-- current dir, sharing deployment.yml & kustomization.yml
- git:
url: git@gitlab.com:
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.
Can you put this into a separate PR so that it properly appears in the changelog later?
remved "draft" prefix, but still want to add some documentation & example. I kept two topics open for @codablock for another round of discussion. |
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.
pkg/deployment/deployment_project.go
Outdated
if item.Path != nil { | ||
// Validate or fix item.Path, as k8s labels has constrains | ||
// regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?') | ||
if *item.Path == "." { |
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.
Can you put this into a separate PR so that it properly appears in the changelog later?
Also, a rebase is required here |
Sure, will check your updates.One think I would like to add it is to have DI.name for even git reference - which either will fix missing include tag for it. item.Path == "." i was actually about tu place it separate. "di" Is nice and for full name we could have an alias. PMi22. 5. 2023 v 16:59, Alexander Block ***@***.***>:
@codablock requested changes on this pull request.
I've create a few commits that I think make sense here:
d8049b0
1bd9651
0cc97f2
The first one already renames the "d" variable to "di", but I'm still unsure if this is the best naming. I'm also considering the fully written version "deploymentItem"...
In pkg/deployment/deployment_project.go:
if item.Path != nil {
+ // Validate or fix item.Path, as k8s labels has constrains
+ // regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')
+ if *item.Path == "." {
Can you put this into a separate PR so that it properly appears in the changelog later?
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
removed, rebased |
Latest commit If the kluctl is in subfolder of the git repo, then the DeployItem relative dir (original variable) is path within the git repo - this was recently introduced as a fix on main branch. which somehow with code: for deployments.yml in PATH/IN/REPO folder:
ends up to upload "compose" to So I had to somehow get the relative only to the project dir - which ended up as new definition for DeploymentProjectVars.TopPath variable. @codablock I am now not any way dependant on this PR. But still seams to me nice feature. You want to make some changes as (vr renames/better path handling). |
Description
This is POC/Draft for discussion only.
Implements #345
Type of change
Please delete options that are not relevant.