feat(Deletion Modes): Populate associated resources field in Manifest #1675
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ate_managedResources_manifest # Conflicts: # api/v1beta2/manifest_types.go # api/v1beta2/zz_generated.deepcopy.go # docs/technical-reference/api/manifest-cr.md
kyma-bot
added
do-not-merge/work-in-progress
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
77cf6d9
to
7eeab44
Compare
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
5608512
to
e1fb17f
Compare
…om/nesmabadr/lifecycle-manager into populate_managedResources_manifest
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
d006b2b
to
3fe52f6
Compare
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
99a0ecd
to
30aea5f
Compare
nesmabadr
added
the
confirm/helm-update
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
30aea5f
to
2da5507
Compare
nesmabadr
changed the title
kyma-bot
added
size/XL
nesmabadr
added
size/L
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
6f858df
to
dc59105
Compare
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
dc59105
to
aef4158
Compare
5 tasks
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
21e7ce2
to
9e1f073
Compare
kyma-bot
added
size/XL
…fest # Conflicts: # internal/manifest/spec_resolver.go # tests/integration/controller/kyma/manifest_test.go
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
9e1f073
to
e8f6fdd
Compare
lindnerby
requested changes
Jul 15, 2024
|
|
||
| "github.com/kyma-project/lifecycle-manager/api/v1beta2" | ||
| "github.com/kyma-project/lifecycle-manager/internal/manifest/filemutex" | ||
| "github.com/kyma-project/lifecycle-manager/pkg/ocmextensions" | ||
| "github.com/kyma-project/lifecycle-manager/pkg/img" |
There was a problem hiding this comment.
Can we move this to internal instead of pkg? Maybe difficult due to usages in template_to_module which should also be moved
| @@ -9,13 +9,31 @@ import ( | |||
|
|
|||
There was a problem hiding this comment.
This package can also go to internal
| CRDsLayer LayerName = "crds" | ||
| ConfigLayer LayerName = "config" | ||
| CRDsLayer LayerName = "crds" | ||
| AssociatedResourcesLayer LayerName = "associated-resources" |
There was a problem hiding this comment.
All of these consts are not used within this package
| @@ -47,3 +58,24 @@ type ( | |||
| ) | |||
|
|
|||
| type Layers []Layer | |||
|
|
|||
| func PullLayer(ctx context.Context, imageRef string, keyChain authn.Keychain) (containerregistryv1.Layer, error) { | |||
| noSchemeImageRef := ocmextensions.NoSchemeURL(imageRef) | |||
There was a problem hiding this comment.
Can be moved down closer to actual usage
| @@ -81,7 +92,7 @@ func (p *Parser) GenerateMandatoryModulesFromTemplates(ctx context.Context, | |||
| return modules | |||
| } | |||
|
|
|||
| func (p *Parser) appendModuleWithInformation(module v1beta2.AvailableModule, kyma *v1beta2.Kyma, | |||
| func (p *Parser) appendModuleWithInformation(ctx context.Context, module v1beta2.AvailableModule, kyma *v1beta2.Kyma, | |||
There was a problem hiding this comment.
Let's refactor this part a bit!
The reason to pass modules list as arg here is to append to it. Appending can be done by consumer, let's change the result of this function to return only the module. and split responsibilities. Then this func also needs a new name.
Also we may try to move all the logic from p.newManifestFromTemplate() into a separate dependency, let's say a ManifestCreator that may be tested separately.
| func (p *Parser) fetchAssociatedResourcesField(ctx context.Context, layer *img.OCI, version string) ([]string, | ||
| error, | ||
| ) { | ||
| associatedResourcesLayer := v1beta2.ImageSpec{ |
There was a problem hiding this comment.
Creating this seems a bit redundant since the only value used for a dependency is layer.CredSecretSelector for the LookupKeyChain()
nesmabadr
force-pushed
the
populate_managedResources_manifest
branch
from
a49921e
to
8d83d22
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Changes proposed in this pull request:
Related issue(s)
Resolves #1651