Flagger addon #440

Eli1123 · 2022-07-20T21:09:15Z

Issue #, if available:

Description of changes:
trying to make a pull request without the history of my past pull request.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

shapirov103 · 2022-07-20T21:12:59Z

package.json

@@ -1,6 +1,6 @@
 {
    "name": "@aws-quickstart/eks-blueprints",
-    "version": "1.0.4",
+    "version": "1.0.5",


shapirov103 · 2022-07-20T21:13:21Z

package.json

@@ -28,6 +28,7 @@
        "typescript": "~4.5.4"
    },
    "dependencies": {
+        "@aws-quickstart/eks-blueprints": "file:/Users/pevetoej/environment/cdk-eks-blueprints/aws-quickstart-eks-blueprints-1.0.5.tgz",


drop this change, it was needed for the patterns repo and we made a mistake to run npm i here.

shapirov103 · 2022-07-20T21:13:38Z

lib/addons/flagger/load-testing-service.yaml

@@ -0,0 +1,15 @@
+apiVersion: v1


please drop all redundant files.

shapirov103

you will need to add addon to the mkdocs.yaml file for navigation.

shapirov103 · 2022-07-25T15:38:50Z

lib/addons/flagger/hpa.yaml

@@ -0,0 +1,21 @@
+apiVersion: autoscaling/v2beta2


what is this file for ? why is it in the source control?

It was in the file example I got this from and figured an example api version in the file wouldn't hurt the test I was using it for. And I have not deleted those temporary yaml files yet, I can do that now.

shapirov103 · 2022-07-25T15:38:59Z

lib/addons/flagger/load-tester.yaml

@@ -0,0 +1,78 @@
+apiVersion: apps/v1


remove this file

shapirov103 · 2022-07-25T15:39:07Z

lib/addons/flagger/podinfo-canary.yaml

@@ -0,0 +1,56 @@
+apiVersion: flagger.app/v1beta1


remove this file

shapirov103 · 2022-07-25T15:39:53Z

lib/addons/flagger/index.ts

+
+    let values: Values = {
+      prometheus: {
+        install: true


you are hardcoding the option that we allows customers to pass, ignoring their setting.

shapirov103 · 2022-07-25T15:40:08Z

lib/addons/flagger/index.ts

+      prometheus: {
+        install: true
+      },
+      meshProvider: MeshProviderOptions.KUBERNETES


Please don't hardcode there values.

I thought the way this worked was it set values to these, then if the user put in their own values values = merge(values, this.props.values ?? {}); would override the hardcoded values I put? I hardcoded them since they are the default if the user did not put in anything was the idea. Or should I not have anything by default?

shapirov103 · 2022-07-25T15:41:39Z

lib/teams/application-team/index.ts

@@ -0,0 +1,11 @@
+import { ArnPrincipal } from 'aws-cdk-lib/aws-iam';


why is this file here?

Oops when I was trying to figure stuff out for patterns and add a team for it I think I put this into the wrong program when I swapped between them.

it is still in the code, why?

It is gone now, guess I got distracted by other issues and never got around to deleting it. I have done it now though.

shapirov103 · 2022-07-25T15:48:53Z

docs/addons/flagger.md

+  .build(app, 'my-stack-name');
+```
+
+## Functionality


do you want to describe supported configuration options?

shapirov103 · 2022-07-25T18:30:41Z

examples/blueprint-construct/index.ts

@@ -5,6 +5,7 @@ import { Construct } from "constructs";
 import * as blueprints from '../../lib';


Fix the GitHub actions warnings.

I am not sure what you mean by that? Are you saying I should have it instead of ../../lib be the import * as blueprints from '@aws-quickstart/eks-blueprints'?

Use file view : https://github.com/aws-quickstart/cdk-eks-blueprints/pull/440/files#diff-40d4a72c979379c5b6ab47139c410af18bd607fd5b4f8a26d1aaef9956d8d447

scroll down from my comment and observe github actions warnings.

…ueprints into flagger-addon

shapirov103

A few comments for you. Mostly minor. The biggest is related to CRD being part of the API. We need to understand its impact.

Desired behavior: CRD must be installed by default.

shapirov103 · 2022-07-26T21:19:54Z

docs/addons/flagger.md

+```typescript
+import * as blueprints from '@aws-quickstart/eks-blueprints';
+
+const flagger = new blueprints.addons.FlaggerAddOn({ 


Why would you provide an example which ignores your own configuration options and leverages values directly? It defeats the purpose.
Please drop the values (that falls under the standard helm add-on configuraton) and provide an example that is using root level properties only (installPrometheus and meshProvider).
With respect to crd.install: you mentioned that it is not needed, and moreover if customers pass it as true stack will fail? Or is it needed?

Well by default Prometheus is true as I stated on line 29 so I figured I would show an example if you wanted to change it. So prometheus has only the option to be false to differ from default, but meshProvider is easier since it has many options. Or is it better to just have the example do FlaggerAddOn() as a pure default example.

And crd is only needed to be true if you have helm v2, if its helm v3 it will break. Should I just remove it all together? Or is that something that should be kept?

We should only demonstrate the use of our root level props and provide a reference to the yaml file if they want to pass something else. It does not make sense to show customers how to ignore your properties and go straight to values. If you want to show usage of values, please pick something that does not overlap with root level properties.

With respect to CRD: we are creating an addon so that customers don't have to use helm. It is irrelevant what version of helm customer installed on their laptop. Our lambda layer is using helm3, that is the only thing that matters here.

Okay then I would say its best to remove it then if that is the case

docs/addons/index.md

shapirov103 · 2022-07-26T21:23:58Z

lib/addons/flagger/index.ts

@@ -0,0 +1,72 @@
+import 'source-map-support/register';


we don't need this import. Why was it added?

Not sure might have been from a quick fix I did but it was not faded so I assumed it was doing something. I have deleted it and see it did not mess anything up.

shapirov103 · 2022-07-26T21:24:34Z

lib/addons/flagger/index.ts

+/**
+ * User provided options for the FlaggerAddonProps values.
+ */
+export interface FlaggerAddOnProps extends blueprints.HelmAddOnUserProps {//this is the root level


please remove the comment // this is the root level
it is evident

That was a personal note I forgot to remove, thanks for the catch.

shapirov103 · 2022-07-26T21:25:50Z

lib/addons/flagger/index.ts

+export interface FlaggerAddOnProps extends blueprints.HelmAddOnUserProps {//this is the root level
+  prometheusInstall?: boolean;
+  meshProvider?: MeshProviderOptions;
+  crd?: boolean;


let's finalize whether we need CRDs to be installed.
simple question: if this is not specified are CRDs installed by default?

If the answer is yes, let's drop it.

That depends on the Helm version the user has. Should we keep it in order to support someone on helm v2 or should we assume all are using helm v3?

shapirov103 · 2022-07-26T21:27:39Z

lib/addons/flagger/index.ts

+
+    let values: Values = {
+      prometheus: {
+        install: this.options.prometheusInstall ?? true


generally we define default values in the defaultProps structure (line 35) as opposed to hardcoding in the code.
so install: this.options.prometheusInstall ?? defaultProps.prometheusInstall

Good point, better to have the code in one place in case they need to be changed and all values are in one place to know what they are for users.

shapirov103 · 2022-07-26T21:28:16Z

lib/teams/application-team/index.ts

@@ -0,0 +1,11 @@
+import { ArnPrincipal } from 'aws-cdk-lib/aws-iam';


it is still in the code, why?

shapirov103 · 2022-07-28T03:33:27Z

@Eli1123 the checks are failing now on make list.

shapirov103

almost there, just a few comments

shapirov103 · 2022-07-28T04:05:03Z

docs/addons/flagger.md

+
+## Functionality
+
+1. Creates the `flagger` namespace.


this parameter is optional and may provided by the user in the namespace field of your addon props.

Changed it to this: 1. Creates the 'flagger' namespace. This parameter is optional and may be provided by the user in the namespace field of your addon props.

shapirov103 · 2022-07-28T04:06:30Z

lib/addons/flagger/index.ts

+ * User provided options for the FlaggerAddonProps values.
+ */
+export interface FlaggerAddOnProps extends blueprints.HelmAddOnUserProps {
+  prometheusInstall?: boolean;


let's rename to installPrometheus to keep it consistent with all flags like that <verb><subject>, e.g.

https://github.com/aws-quickstart/cdk-eks-blueprints/blob/main/lib/addons/appmesh/index.ts#L17

https://github.com/aws-quickstart/cdk-eks-blueprints/blob/main/lib/addons/cluster-autoscaler/index.ts#L24

That makes sense.

shapirov103 · 2022-07-28T04:09:25Z

examples/blueprint-construct/index.ts

@@ -20,7 +20,7 @@ export interface BlueprintConstructProps {
 export default class BlueprintConstruct {
    constructor(scope: Construct, props: cdk.StackProps) {

-        HelmAddOn.validateHelmVersions = true;
+        //HelmAddOn.validateHelmVersions = true;


please fix the three github actions warning below like 'teams' is assigned a value but never used

Should I revert this file back to what it was before since I was using it for testing purposes?

Yes, please revert.

shapirov103

Almost there. some minor comments on code style.

shapirov103 · 2022-07-28T20:57:47Z

lib/addons/flagger/index.ts

@@ -0,0 +1,66 @@
+import { HelmAddOn, HelmAddOnUserProps, HelmAddOnProps } from "../helm-addon";
+import { ClusterInfo } from "../../spi";


please group import together. So whatever is coming from ../../spi should be a single import.

Oh did not notice I did that.

lib/addons/flagger/index.ts

shapirov103 · 2022-07-28T20:58:46Z

lib/addons/flagger/index.ts

+ * This creates and deploys a cluster with the FlaggerAddOnProps values for flagger settings with preset values unless the user specifies their own values.
+ */
+export class FlaggerAddOn extends HelmAddOn{
+  readonly options: FlaggerAddOnProps;


line break here.
HelmAddOn{Space}{
line breaks after class declaration, after member declaration, method. Inside methods if there are distinct paragraphs like {init block}, {call this API block}, {if/then/else} block, use line breaks to segregate.

shapirov103 · 2022-07-28T20:58:50Z

lib/addons/flagger/index.ts

+ */
+export class FlaggerAddOn extends HelmAddOn{
+  readonly options: FlaggerAddOnProps;
+  constructor(props?: FlaggerAddOnProps) {


line break here.

shapirov103 · 2022-07-28T20:58:55Z

lib/addons/flagger/index.ts

+    super({ ...defaultProps, ...props });
+    this.options = this.props as FlaggerAddOnProps;
+  }
+  deploy(clusterInfo: ClusterInfo): Promise<Construct> {


line break here.

shapirov103 · 2022-07-28T21:29:21Z

lib/addons/flagger/index.ts

+ * User provided options for the FlaggerAddonProps values.
+ */
+export interface FlaggerAddOnProps extends HelmAddOnUserProps {
+


public docs for public members of the interface.

I am unsure what you are asking for by that.

Docs for public members of the interface. Jsdocs.

I don't believe I've done a jsdoc yet, I'll have to look into it. Or is that what the @ comments were called.

Like this? (The * are translated into bullets in this text box)
/**

User provided options for the FlaggerAddonProps values.

@extends {HelmAddOnUserProps} is needed to set Helm chart values for the interface.

@prop {boolean} "determines if prometheus is installed with the addon."

@prop {MeshProviderOptions} "Determines what value for mesh provider is used."
*/
export interface FlaggerAddOnProps extends HelmAddOnUserProps {

installPrometheus?: boolean;
meshProvider?: MeshProviderOptions;
}

Just regular jsdocs on the public member. That means doc above installPrometheus and doc above meshProvider so that when developers use your addon, they will see what it means.

so more like this?

/**

installPrometheus controls if prometheus is installed with the addon or not.
/
installPrometheus?: boolean;
/*

meshProvider controls what mesh provider from MeshProviderOptions enums list is used.
*/
meshProvider?: MeshProviderOptions;

Please don't repeat the name of the field in the doc.

/** * Controls if prometheus is installed with the addon or not. * default: true */ installPrometheus?: boolean;

That makes sense, so more like this:
/**

Controls if prometheus is installed with the addon or not.

default: true
/
installPrometheus?: boolean;
/*

Controls what mesh provider from MeshProviderOptions enums list is used.

default: MeshProviderOptions.KUBERNETES
*/
meshProvider?: MeshProviderOptions;

Yes. Note the formatting of comments.

shapirov103 · 2022-07-28T21:30:17Z

lib/addons/flagger/index.ts

+  release: "flagger",
+  repository: "https://flagger.app",
+  values: {
+


this space is not needed, it is a json structure, not code.

shapirov103 · 2022-07-28T21:30:44Z

lib/addons/flagger/index.ts

+  deploy(clusterInfo: ClusterInfo): Promise<Construct> {
+
+    let values: Values = {
+


don't break json structures. They are not code.

elamaran11 · 2023-08-30T16:54:14Z

@Eli1123 Are you planning to work on this PR?

github-actions · 2023-11-29T00:04:33Z

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

github-actions · 2024-02-28T00:03:28Z

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

elamaran11 · 2024-03-07T15:10:00Z

@Eli1123 Are you planning to work on this PR?

github-actions · 2024-06-06T00:04:29Z

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

Eli1123 · 2024-07-01T19:04:18Z

@Eli1123 Are you planning to work on this PR?

Sorry, missed this. Currently not planning to continue my intern project here as other projects are keeping me busy. Perhaps later down the road but if someone else wishes to pick up where I left off in the mean time they can feel free to do so.

github-actions · 2024-09-30T00:05:05Z

This PR has been automatically marked as stale because it has been open 60 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

Eli1123 added 7 commits July 18, 2022 17:06

addon start

25a5798

Lets see if this updates

85c3fd9

minor work on flagger

159cb5a

testing commit while on different branch

050652a

remove canary for real now

81916a4

minor code progress

9e1705c

minor update

9d2db1f

Eli1123 requested review from aws-ia-ci and a team as code owners July 20, 2022 21:09

shapirov103 reviewed Jul 20, 2022

View reviewed changes

Eli1123 added 3 commits July 20, 2022 16:57

bla bla bla

79135f2

progress on bootstrap

2cca7a8

Flagger addon almost done, with docs

39215e8

shapirov103 reviewed Jul 25, 2022

View reviewed changes

Eli1123 added 2 commits July 25, 2022 16:25

Mostly done flagger addon, and docs

8459432

Merge branch 'flagger-addon' of https://github.com/Eli1123/cdk-eks-bl…

4ac7046

…ueprints into flagger-addon

shapirov103 reviewed Jul 26, 2022

View reviewed changes

Removed CRD and added minor changes

5768589

shapirov103 reviewed Jul 28, 2022

View reviewed changes

Eli1123 and others added 3 commits July 28, 2022 12:17

minor changes

4361c80

fixed cdk list issue

4efc9bb

Merge branch 'aws-quickstart:main' into flagger-addon

1a1a313

shapirov103 reviewed Jul 28, 2022

View reviewed changes

Eli1123 added 2 commits July 28, 2022 16:08

Fixed code line spacing

12da84c

more spacing in code

795899c

shapirov103 reviewed Jul 28, 2022

View reviewed changes

Eli1123 and others added 2 commits July 29, 2022 10:52

some spacing, and comment changes

6a788b2

Merge branch 'aws-quickstart:main' into flagger-addon

3b7a146

github-actions bot added the stale label Nov 29, 2023

elamaran11 removed the stale label Nov 29, 2023

github-actions bot added the stale label Feb 28, 2024

shapirov103 added add-on and removed stale labels Feb 28, 2024

github-actions bot added the stale label Jun 6, 2024

elamaran11 removed the stale label Jun 6, 2024

github-actions bot added the stale label Sep 30, 2024

		@@ -0,0 +1,11 @@
		import { ArnPrincipal } from 'aws-cdk-lib/aws-iam';

		@@ -5,6 +5,7 @@ import { Construct } from "constructs";
		import * as blueprints from '../../lib';

		@@ -0,0 +1,66 @@
		import { HelmAddOn, HelmAddOnUserProps, HelmAddOnProps } from "../helm-addon";
		import { ClusterInfo } from "../../spi";

		deploy(clusterInfo: ClusterInfo): Promise<Construct> {

		let values: Values = {

Flagger addon #440

Are you sure you want to change the base?

Flagger addon #440

Conversation

Eli1123 commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shapirov103 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

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

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eli1123 Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

shapirov103 Jul 27, 2022 • edited Loading

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

shapirov103 commented Jul 28, 2022

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eli1123 Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

shapirov103 Jul 28, 2022 • edited Loading

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

shapirov103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shapirov103 Jul 28, 2022 • edited Loading

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

Eli1123 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Eli1123 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shapirov103 Jul 29, 2022 • edited Loading

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

elamaran11 commented Aug 30, 2023

github-actions bot commented Nov 29, 2023

github-actions bot commented Feb 28, 2024

Eli1123 Jul 27, 2022 •

edited

Loading

shapirov103 Jul 27, 2022 •

edited

Loading

Eli1123 Jul 28, 2022 •

edited

Loading

shapirov103 Jul 28, 2022 •

edited

Loading

shapirov103 Jul 28, 2022 •

edited

Loading

Eli1123 Jul 29, 2022 •

edited

Loading

Eli1123 Jul 29, 2022 •

edited

Loading

shapirov103 Jul 29, 2022 •

edited

Loading