Refactor and simplify structs (#2314) #20

keithchong · 2022-09-06T19:19:06Z

The initial HAS struct types were maintained. The goal now is to simplify, especially when the ComponentSpec configuration and the BindingComponentConfiguration have overlap. (They are both used primarily in the generation of the basic k8s deployment.yaml).

Each element in the BindingComponentConfiguration is mapped nicely to the ComponentSpec, except for Resources, where in the former, the ResourceRequirement is a pointer, but, in the latter, it is a non-pointer. This PR will use a non-pointer. Similarly, this reflects how it is done in k8s Container. See
https://github.com/kubernetes/api/blob/6dd661f592205cc6035520138ebdbd271b9fa197/core/v1/types.go#L2369

The goal is ‘flatten’ the structs and simplify, so the elements from ComponentSpec will be moved to the Component struct. The Source element will have the ComponentSource type however, and these types are flattened too. (There isn’t a need for multiple GitSources and Unions so this PR simplifies this.)

Mapping:
Original: component.Spec.Source.ComponentSourceUnion.GitSource, where GitSource is a pointer to GitSource
New: component.Source.ComponentSource, where Source is a pointer to ComponentSource

This is so that you can check for a nil ComponentSource. See the corresponding changes in the Application Service PR. This is similar to checking if component.Spec.Source.ComponentSourceUnion.GitSource != nil

Signed-off-by: Keith Chong kykchong@redhat.com

keithchong · 2022-09-06T21:00:24Z

Changes to HAS are in this branch, excluding the go.mod file (just pull in the changes from this PR to see)

https://github.com/keithchong/application-service/tree/2314-ReactToRefactoredStructsFromGitOpsGenerator

keithchong · 2022-09-07T18:14:09Z

Here is the commit (differences) in HAS:

redhat-appstudio/application-service@e96d0e2

pkg/gitops.go

kim-tsao · 2022-09-09T16:59:42Z

api/v1alpha1/component_types.go

@@ -38,18 +39,11 @@ type GitSource struct {
 	DockerfileURL string `json:"dockerfileUrl,omitempty"`


Is there an order of precedence for component creation if the URL, DevfileURL, and DockerfileURLs are all specified at once? Should this be documented?

Right now, they are placeholders. Those are currently not used by the Generator - only the URL is used.

Based on your later review comments, I'll get rid of these.

I don't know if this will break HAS. If these are HAS specific, then they belong in the HAS repo. They don't really fit the category of "git source" either. cc @maysunfaisal

pkg/gitops_test.go

api/v1alpha1/component_types.go

maysunfaisal · 2022-09-09T17:45:34Z

api/v1alpha1/component_types.go

@@ -62,103 +56,27 @@ type ComponentSpec struct {
 	Secret string `json:"secret,omitempty"`

 	// Source describes the Component source
-	Source ComponentSource `json:"source,omitempty"`
+	Source *ComponentSource `json:"source,omitempty"`


how about

Suggested change

Source *ComponentSource `json:"source,omitempty"`

GitSource *GitSource `json:"gitSource,omitempty"`

because ContainerImage can also be a source. This would mean also renaming type ComponentSource struct{}

I am thinking about making this change in the application-service too after thinking about it now. it looks cleaner

api/v1alpha1/component_types.go

keithchong · 2022-09-28T18:04:52Z

Note that the definition of Resource is different in GitOps service and HAS:

Managed GitOps Services

https://github.com/redhat-appstudio/managed-gitops/blob/864c0c9c5a743275a4b0e497654a9220a4fb993f/appstudio-shared/apis/appstudio.redhat.com/v1alpha1/applicationsnapshotenvironmentbinding_types.go#L67

HAS

https://github.com/redhat-appstudio/application-service/blob/0b7dd2d0dc4d9a31c6803a9bbbab2b985ddbbcdf/api/v1alpha1/component_types.go#L89

Both are used when generating the deployment file (deployment kind). I've changed it to be a non-pointer. See the component_types.go file.

keithchong · 2022-09-28T18:14:42Z

Here is the updated commit in HAS to reflect these changes

redhat-appstudio/application-service@93fd73a

keithchong · 2022-09-29T13:30:19Z

Here is the latest commit for the HAS repo to reflect these changes. A follow-on PR for that will be created including updating the go.mod file, etc. (It just has the source changes)

redhat-appstudio/application-service@a951ab0

maysunfaisal

Just a suggestion to change function param to options since we changed the struct name
pls have a look at my ENV suggestion for deployment patch func, is it valid?

maysunfaisal · 2022-10-11T20:46:17Z

api/v1alpha1/component_types.go

 type GitSource struct {
 	// If importing from git, the repository to create the component from
 	URL string `json:"url"`

 	// Specify a branch/tag/commit id. If not specified, default is `main`/`master`.
-	Revision string `json:"revision,omitempty"`
+	//Revision string `json:"revision,omitempty"`


I think we might need to keep this

These are not used anywhere in the source so far.

It is used in the application-service repo which will eventually rely on this code (see https://github.com/redhat-appstudio/application-service/blob/651fc2e107c413f88dc3796b581ef13e3387243c/controllers/component_controller.go#L210)

That is application-service's original types. Note that a duplicate of these types (structs) were made in the common library to make the transition to the library easier. This duplicate or the 'mapping' now needs to be reduced because many of the fields are not necessary when generating the k8s resources. If you need any field added to the 'mapping', then that field must be used by the library in some way. Currently, what you see now in this PR are the fields that are used.

pkg/generate.go

pkg/gitops.go

keithchong · 2022-10-12T21:19:13Z

Just a suggestion to change function param to options since we changed the struct name

pls have a look at my ENV suggestion for deployment patch func, is it valid?

I also renamed the file in api/v1alpha1 from component_types.go to generator_options.go

codecov-commenter · 2022-10-12T21:20:43Z

Codecov Report

Base: 81.63% // Head: 80.67% // Decreases project coverage by -0.96% ⚠️

Coverage data is based on head (42a9ef1) compared to base (ad5e970).
Patch coverage: 88.40% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #20      +/-   ##
==========================================
- Coverage   81.63%   80.67%   -0.97%     
==========================================
  Files           6        6              
  Lines         588      595       +7     
==========================================
  Hits          480      480              
- Misses         99      106       +7     
  Partials        9        9

Impacted Files	Coverage Δ
pkg/gitops.go	`57.78% <55.55%> (-2.53%)`	⬇️
pkg/generate.go	`90.54% <100.00%> (+0.06%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

api/v1alpha1/generator_options.go

keithchong · 2022-10-13T20:09:33Z

Here are the latest rebased changes to the application-service to react to these changes (minus the go.mod changes)

redhat-appstudio/application-service@94e3c99

Signed-off-by: Keith Chong <kykchong@redhat.com>

keithchong · 2022-10-13T22:40:44Z

Thanks for the review folks. We needed to rebase again.

keithchong requested review from maysunfaisal and johnmcollier September 6, 2022 19:19

kim-tsao reviewed Sep 9, 2022

View reviewed changes

pkg/gitops.go Outdated Show resolved Hide resolved

kim-tsao reviewed Sep 9, 2022

View reviewed changes

pkg/gitops_test.go Show resolved Hide resolved

kim-tsao reviewed Sep 9, 2022

View reviewed changes

pkg/gitops_test.go Show resolved Hide resolved

kim-tsao reviewed Sep 9, 2022

View reviewed changes

pkg/gitops_test.go Show resolved Hide resolved

maysunfaisal reviewed Sep 9, 2022

View reviewed changes

api/v1alpha1/component_types.go Outdated Show resolved Hide resolved

maysunfaisal reviewed Sep 9, 2022

View reviewed changes

keithchong force-pushed the 2314-RefactorStructs branch from 4cf3290 to 11fa47f Compare September 28, 2022 18:02

keithchong force-pushed the 2314-RefactorStructs branch from 11fa47f to 315d3da Compare September 28, 2022 22:30

maysunfaisal reviewed Oct 11, 2022

View reviewed changes

keithchong force-pushed the 2314-RefactorStructs branch 2 times, most recently from 79f1b38 to 48d8f3f Compare October 12, 2022 21:18

maysunfaisal reviewed Oct 12, 2022

View reviewed changes

api/v1alpha1/generator_options.go Outdated Show resolved Hide resolved

keithchong force-pushed the 2314-RefactorStructs branch from 48d8f3f to 7b7ab8c Compare October 13, 2022 20:07

maysunfaisal approved these changes Oct 13, 2022

View reviewed changes

keithchong force-pushed the 2314-RefactorStructs branch from 7b7ab8c to 336737c Compare October 13, 2022 21:44

maysunfaisal approved these changes Oct 13, 2022

View reviewed changes

Refactor and simplify structs (#2314)

42a9ef1

Signed-off-by: Keith Chong <kykchong@redhat.com>

keithchong force-pushed the 2314-RefactorStructs branch from ee1f1ff to 42a9ef1 Compare October 13, 2022 22:36

maysunfaisal approved these changes Oct 13, 2022

View reviewed changes

keithchong merged commit 8f34bdc into redhat-developer:main Oct 13, 2022

keithchong mentioned this pull request Oct 14, 2022

React to refactored structs in GitOps Generator (#2314) redhat-appstudio/application-service#200

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and simplify structs (#2314) #20

Refactor and simplify structs (#2314) #20

keithchong commented Sep 6, 2022

keithchong commented Sep 6, 2022

keithchong commented Sep 7, 2022

kim-tsao Sep 9, 2022

keithchong Sep 26, 2022

keithchong Sep 26, 2022

kim-tsao Sep 29, 2022

maysunfaisal Sep 9, 2022

keithchong commented Sep 28, 2022

keithchong commented Sep 28, 2022

keithchong commented Sep 29, 2022 •

edited

Loading

maysunfaisal left a comment

maysunfaisal Oct 11, 2022

keithchong Oct 12, 2022

kim-tsao Oct 12, 2022

keithchong Oct 12, 2022 •

edited

Loading

keithchong commented Oct 12, 2022

codecov-commenter commented Oct 12, 2022 •

edited

Loading

keithchong commented Oct 13, 2022

keithchong commented Oct 13, 2022

		@@ -38,18 +39,11 @@ type GitSource struct {
		DockerfileURL string `json:"dockerfileUrl,omitempty"`

	Source *ComponentSource `json:"source,omitempty"`
	GitSource *GitSource `json:"gitSource,omitempty"`

Refactor and simplify structs (#2314) #20

Refactor and simplify structs (#2314) #20

Conversation

keithchong commented Sep 6, 2022

keithchong commented Sep 6, 2022

keithchong commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithchong commented Sep 28, 2022

keithchong commented Sep 28, 2022

keithchong commented Sep 29, 2022 • edited Loading

maysunfaisal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithchong Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

keithchong commented Oct 12, 2022

codecov-commenter commented Oct 12, 2022 • edited Loading

Codecov Report

keithchong commented Oct 13, 2022

keithchong commented Oct 13, 2022

keithchong commented Sep 29, 2022 •

edited

Loading

keithchong Oct 12, 2022 •

edited

Loading

codecov-commenter commented Oct 12, 2022 •

edited

Loading