Add item list CRD Spec #1

xianlubird · 2020-06-05T04:08:44Z

# The list generator specifies a literal list of argument values to the app spec template.
apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: guestbook
spec:
  generators:
  - list:
      items:
      - cluster: engineering-dev
        url: https://1.2.3.4
      - cluster: engineering-prod
        url: https://2.4.6.8
      - cluster: finance-preprod
        url: https://9.8.7.6
  template:
    metadata:
      name: '{{name}}-guestbook'
    spec:
      source:
        repoURL: https://github.com/infra-team/cluster-deployments.git
        targetRevision: HEAD
        path: guestbook/{{cluster}}
      destination:
        server: '{{url}}'
        namespace: guestbook

First crd implement list item function

CLAassistant · 2020-06-05T04:08:49Z

All committers have signed the CLA.

dgoodwin · 2020-06-08T11:28:26Z

Makefile

+
+.PHONY: image
+image:
+	docker build -t registry.cn-hangzhou.aliyuncs.com/appcenter/argocd-aplicationset:$(IMAGE_TAG) .


Could we make the registry configurable as well, $IMAGE perhaps?

I agree. Can make it such that it builds a local image (without a registry), but will prepend IMAGE_PREFIX if it is defined?

jessesuen · 2020-06-08T23:08:44Z

Makefile

+
+.PHONY: image
+image:
+	docker build -t registry.cn-hangzhou.aliyuncs.com/appcenter/argocd-aplicationset:$(IMAGE_TAG) .


I agree. Can make it such that it builds a local image (without a registry), but will prepend IMAGE_PREFIX if it is defined?

jessesuen · 2020-06-08T23:14:22Z

api/v1alpha1/applicationset_types.go

+type ApplicationSetSpec struct {
+	Generators ApplicationSetGenerators  `json:"generators"`
+	Template   ApplicationSetTemplate    `json:"template"`
+	Operation  *v1alpha1.SyncOperation   `json:"operation,omitempty"`


Regarding the operation field:

Reusing the SyncOperation from the Applications APIs probably doesn't make sense here, since that object deals with a single git revision. But ApplicationSets may deal with many revisions, and even many repositories. If we do decide to have operation, we will need a new data structure.

In fact, we're not sure what a sync operation even means for an AppSet. Currently, I think it would only be used as a convenience field which carries forward setting the operation field for all the Applications in the ApplicationSet. But this could be done as a convenience in the API server instead.

We've had some regrets about introducing this operation field in Applications, since a better alternative might have been to introduce an SyncOperation CRD and controller to perform the logic. This CRD would act like a Job object and finish Successfully or Failed.

So with the above, I think we should leave out operation, until we think about how we want to do operations (if at all).

jessesuen · 2020-06-08T23:29:53Z

api/v1alpha1/applicationset_types.go

+type SyncPolicyAutomated struct {
+	// Prune will prune resources automatically as part of automated sync (default: false)
+	Prune       bool `json:"prune,omitempty"`
+	InitialSync bool `json:"initialSync,omitempty"`


I'm thinking the word sync is redundant in the initialSync, since it's already part of a syncPolicy. How about:

spec: syncPolicy: automated: prune: true initial: true

Let's also comment this to explain what this option is for:

// Initial will perform an initial sync for any newly created Applications which do not have automated sync turned on.

dgoodwin

Just a couple quick thoughts on naming the structs, please disregard if this doesn't look better to you.

dgoodwin · 2020-06-11T12:10:42Z

api/v1alpha1/applicationset_types.go

+}
+
+// GeneratorsList include items info
+type GeneratorsList struct {


GeneratorsList implies this is a list of all generators, what do you think about ListGenerator to more clearly indicate what this is?

dgoodwin · 2020-06-11T12:10:57Z

api/v1alpha1/applicationset_types.go

+}
+
+// GeneratorsItems include cluster and url info
+type GeneratorsItems struct {


Suggest ListGeneratorItem

dgoodwin · 2020-06-11T12:16:27Z

api/v1alpha1/applicationset_types.go

+
+// ApplicationSetGenerators include list item info
+type ApplicationSetGenerators struct {
+	List GeneratorsList `json:"list, omitempty"`


Feels like this should be *GeneratorsList as it is optional.

dgoodwin · 2020-06-11T12:46:05Z

main.go

+
+	if err = (&controllers.ApplicationSetReconciler{
+		Client: mgr.GetClient(),
+		Log:    ctrl.Log.WithName("controllers").WithName("ApplicationSet"),


Minor compile error here as the reconciler has no Log field.

jessesuen · 2020-06-12T03:13:22Z

api/v1alpha1/applicationset_types.go

+
+// ApplicationSetSpec represents a class of application set state.
+type ApplicationSetSpec struct {
+	Generators ApplicationSetGenerators  `json:"generators"`


Per spec, it should be an array of ApplicationSetGenerator:

spec: generators: - clusters: {}

Generators []ApplicationSetGenerator `json:"generators"`

jessesuen · 2020-06-12T03:14:33Z

api/v1alpha1/applicationset_types.go

+}
+
+// ApplicationSetGenerators include list item info
+type ApplicationSetGenerators struct {


We should remove s from ApplicationSetGenerators since this should be a single generator

type ApplicationSetGenerator struct { List *ListGenerator `json:"list, omitempty"` }

Also, we should remove s from ListGenerators as well.

jessesuen · 2020-06-12T03:15:28Z

api/v1alpha1/applicationset_types.go

+}
+
+// ItemGenerators include cluster and url info
+type ItemGenerators struct {


type ListGeneratorItem struct

jessesuen · 2020-06-12T03:16:18Z

api/v1alpha1/applicationset_types.go

+}
+
+// ListGenerators include items info
+type ListGenerators struct {


Should be:

type ListGenerator struct {

(no s)

jessesuen · 2020-06-12T03:20:37Z

api/v1alpha1/applicationset_types.go

+// ApplicationSetTemplate represents argocd ApplicationSpec
+type ApplicationSetTemplate struct {
+	metav1.ObjectMeta `json:"metadata"`
+	TemplateSpec      v1alpha1.ApplicationSpec `json:"spec"`


I think the field name needs to be named Spec, otherwise this may be an K8s API violation since the golang field name does not match the json field name.

Add item list CRD Spec

61e0591

xianlubird requested a review from jessesuen June 5, 2020 04:09

Add manifest

fbbe8c0

dgoodwin reviewed Jun 8, 2020

View reviewed changes

jessesuen reviewed Jun 8, 2020

View reviewed changes

Update makefile and struct field as comments

f3ff421

xianlubird force-pushed the list-function branch from 4a34567 to f3ff421 Compare June 11, 2020 02:29

xianlubird requested review from dgoodwin and jessesuen June 11, 2020 02:31

dgoodwin reviewed Jun 11, 2020

View reviewed changes

dgoodwin mentioned this pull request Jun 11, 2020

Sketch out cluster generator in the CRD, and generator interface #2

Merged

Update as comments

1107612

xianlubird requested a review from dgoodwin June 12, 2020 02:59

jessesuen reviewed Jun 12, 2020

View reviewed changes

update as comments

24566e8

jessesuen approved these changes Jun 12, 2020

View reviewed changes

xianlubird merged commit 24cdcc2 into argoproj:master Jun 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add item list CRD Spec #1

Add item list CRD Spec #1

xianlubird commented Jun 5, 2020

CLAassistant commented Jun 5, 2020 •

edited

dgoodwin Jun 8, 2020

jessesuen Jun 8, 2020

xianlubird Jun 11, 2020

jessesuen Jun 8, 2020

jessesuen Jun 8, 2020

xianlubird Jun 11, 2020

jessesuen Jun 8, 2020

xianlubird Jun 11, 2020

dgoodwin left a comment

dgoodwin Jun 11, 2020

xianlubird Jun 12, 2020

dgoodwin Jun 11, 2020

dgoodwin Jun 11, 2020

xianlubird Jun 12, 2020

dgoodwin Jun 11, 2020

xianlubird Jun 12, 2020

jessesuen Jun 12, 2020

jessesuen Jun 12, 2020

jessesuen Jun 12, 2020

jessesuen Jun 12, 2020

jessesuen Jun 12, 2020 •

edited

Add item list CRD Spec #1

Add item list CRD Spec #1

Conversation

xianlubird commented Jun 5, 2020

CLAassistant commented Jun 5, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgoodwin 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessesuen Jun 12, 2020 • edited

Choose a reason for hiding this comment

CLAassistant commented Jun 5, 2020 •

edited

jessesuen Jun 12, 2020 •

edited