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

Porch PackageVariant Kptfile pipeline editing feature #117

Closed
johnbelamaric opened this issue Apr 13, 2023 · 8 comments · Fixed by kptdev/kpt#3925
Closed

Porch PackageVariant Kptfile pipeline editing feature #117

johnbelamaric opened this issue Apr 13, 2023 · 8 comments · Fixed by kptdev/kpt#3925
Assignees
Labels
area/package-management SIG Automation Package Management Subproject sig/automation
Milestone

Comments

@johnbelamaric
Copy link
Member

Part of the PV design in #36 - see https://github.com/GoogleContainerTools/kpt/blob/main/docs/design-docs/08-package-variant.md#krm-function-pipeline1

@johnbelamaric johnbelamaric added area/package-management SIG Automation Package Management Subproject sig/automation labels Apr 13, 2023
@johnbelamaric johnbelamaric added this to the sprint2 milestone Apr 13, 2023
@johnbelamaric johnbelamaric moved this to Todo in Nephio R1 Apr 13, 2023
@johnbelamaric johnbelamaric moved this from Todo to In Progress in Nephio R1 Apr 13, 2023
@gvbalaji gvbalaji modified the milestones: sprint2, sprint3 Apr 25, 2023
@SimonTheLeg
Copy link
Member

Short update on this. In order to merge kptdev/kpt#3925 in an elegant way, the following two upstream contributions are required:

@SimonTheLeg
Copy link
Member

SimonTheLeg commented Apr 26, 2023

So short update on this. It turns out the kubernetes-sigs cli-utils is dependent on another upstream component, controller-runtime, which then is dependent on a release of apimachinery. This brings our total upstream "mergetrain" to this:

Long story short: At this point it is difficult to assess how long it will take for upstream to finish. And I would like to avoid stepping even deeper into the rabbit hole.

Therefore, tomorrow I will work on addressing the open topics from @johnbelamaric's review. This then will either be using it without deepcopy or I go into Mortens idea of the kptfile being a string.

@johnbelamaric
Copy link
Member Author

Any updates here, @SimonTheLeg ?

@SimonTheLeg
Copy link
Member

Unfortunately not. I am really struggling with the editing via kyaml. I have it now to a point where it is possible to append to an existing map and it will keep the seqIndentStyle, but now it is struggling with appending if the list was empty before. In a nutshell: either I am missing something quite obvious or this is really complicated to do. In any case: I'll give it another shot today and will push what I have at the end of the day. And then we can have a look either after sig-automation today or pkg-mgmt subproject tomorrow

@johnbelamaric
Copy link
Member Author

Are you using the kpt function SDK? It makes this a lot easier, once you understand it. Take a look at how I did the ConfigMap changes in the package context code, or later today I can provide an example another PR I am working on.

@SimonTheLeg
Copy link
Member

So I have pushed something now that is working using kyaml that has the same features as the solution before. I'll have to add the validators and idempotency next.


Are you using the kpt function SDK?

That is a good point. I was not yet. I had a look now, but the main challenges for me so far were the reverse appending to a yaml sequence and keeping the comments. And at least from a first glance I don't think kpt fn sdk helps with that. Also in the packageContextCode I noticed:

@johnbelamaric
Copy link
Member Author

Yeah, good point about the comments. @kispaljr I believe has an update to the SDK that preserves comments better. We can get that integrated at some point. I am OK with forgoing that and treating it as an SDK bug that needs fixing, for now. In other words, I would just go ahead and use the SDK for now. You are right the way I do it in the link is not the best, for these reasons, and also should be fixed. I am more interested in getting the critical path going; we can perfect it later (though we should capture it in an issue).

@johnbelamaric
Copy link
Member Author

This PR may be helpful to you as well: kptdev/kpt#3940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/package-management SIG Automation Package Management Subproject sig/automation
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants