feat(helm): Use commandArgs for all command arguments and add dlv debug helm test #484

mikesmithgh · 2022-03-25T16:47:30Z

Merge after PR: #483 is merged to master

codecov · 2022-03-25T16:52:23Z

Codecov Report

Merging #484 (9be8f2b) into master (3658206) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #484   +/-   ##
=======================================
  Coverage   49.33%   49.33%           
=======================================
  Files          57       57           
  Lines        5021     5021           
=======================================
  Hits         2477     2477           
  Misses       2290     2290           
  Partials      254      254

Flag	Coverage Δ
integration	`43.33% <ø> (+0.10%)`	⬆️
unit	`28.12% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3658206...9be8f2b. Read the comment docs.

mikesmithgh · 2022-03-29T13:23:11Z

deploy/helm/metacontroller/templates/statefulset.yaml

-          {{- if .Values.cacheFlushInterval -}}
-          - --cache-flush-interval={{.Values.cacheFlushInterval}}
-          {{ end -}}
+          # TODO: discuss, these args will always be passed causing dlv to fail.


TODO: discuss, these args will always be passed causing dlv to fail.
I believe we should simpy use commandArgs and remove all these flags but this is a breaking change (leaning toward this option to simplify) or update the values such that they are optionally applied

I updated this PR to consolidate all metacontroller cli args as commandArgs. This is a breaking change.

grzesuav · 2022-03-30T17:15:28Z

Dockerfile.debug

@@ -9,6 +9,7 @@ WORKDIR /go/src/metacontroller/
 ENV ADDITIONAL_BUILD_ARGUMENTS='-gcflags="all=-N -l"'
 RUN make install
 RUN go get github.com/go-delve/delve/cmd/dlv
+RUN go install github.com/go-delve/delve/cmd/dlv


hmm I think line above is doing the same...

@grzesuav It looks like the functionality changed in go 1.18: https://tip.golang.org/doc/go1.18#modules

go get
go get no longer builds or installs packages in module-aware mode. go get is now dedicated to adjusting dependencies in go.mod. Effectively, the -d flag is always enabled. To install the latest version of an executable outside the context of the current module, use go install example.com/cmd@latest. Any version query may be used instead of latest. This form of go install was added in Go 1.16, so projects supporting older versions may need to provide install instructions for both go install and go get. go get now reports an error when used outside a module, since there is no go.mod file to update. In GOPATH mode (with GO111MODULE=off), go get still builds and installs packages, as before.

I was confused by this as well 😂 . I updated it just use the install command now.

grzesuav · 2022-03-30T17:15:55Z

.github/workflows/test_release.yaml

@@ -127,15 +127,23 @@ jobs:
            echo "::set-output name=changed::true"
          fi

-      - name: Setup kind
+      - name: Build local debug image


why you would like to use debug image in testing ?

For this test I want to cover two things.

The helm chart can accept values to allow deploying the debug version of metacontroller

Any helm chart or Dockerfile changes do not break the debug version of metacontroller

Basically, this is more a dev facing use case than user facing. Please let me know if this makes sense.

mikesmithgh · 2022-04-11T13:54:31Z

Note to self:

Use https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts to share docker image across jobs in workflow

Upload as tar
Download tar and docker load
Then kind load

Set small retention policy

mikesmithgh · 2022-04-12T12:39:15Z

.github/workflows/test_release.yaml

@@ -2,6 +2,28 @@
 name: Test / Release
 on: [push, pull_request]
 jobs:
+  dockerbuild:


This lets us build the docker image once and reference it in each kind version rather than rebuilding the docker image for every kind version.

mikesmithgh · 2022-04-12T12:42:11Z

deploy/helm/metacontroller/ci/debug-values.yaml

@@ -4,7 +4,7 @@ rbac:
 image:


I removed pdb-value.yaml as the test coverage for pdb is included in leader-election-values.yaml as well. (I'm not sure why github thinks this was a rename)

mikesmithgh · 2022-04-12T12:52:12Z

Hi @grzesuav , this PR is ready for review. Please let me know your feedback. Also, this has a breaking change. I included the BREAKING CHANGE footer in the commit message so hopefully it versions properly (first time committing a breaking change with semantic release for me).

mikesmithgh · 2022-04-12T13:05:33Z

@grzesuav Also, I created #509 for flakiness in the decorator controller integration test.

Signed-off-by: Mike Smith <10135646+mjsmith1028@users.noreply.github.com>

BREAKING CHANGE: The following helm values are removed. The equivalent command arguments can now be passed directly to the `commandArgs` value. - discoveryInterval - cacheFlushInterval - zap.logLevel - zap.devel - zap.encoder - zap.stacktraceLevel Signed-off-by: Mike Smith <10135646+mjsmith1028@users.noreply.github.com>

grzesuav · 2022-05-04T16:56:07Z

🎉 This PR is included in version 4.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

mikesmithgh force-pushed the helm-values-command branch 3 times, most recently from 15e5176 to 97a459d Compare March 25, 2022 17:32

mikesmithgh commented Mar 29, 2022

View reviewed changes

grzesuav reviewed Mar 30, 2022

View reviewed changes

mikesmithgh force-pushed the helm-values-command branch from 97a459d to 65c0aa0 Compare April 10, 2022 00:18

mikesmithgh mentioned this pull request Apr 10, 2022

Helm chart improvements #302

Closed

mikesmithgh force-pushed the helm-values-command branch 4 times, most recently from 6040156 to 8f29fac Compare April 11, 2022 13:21

mikesmithgh force-pushed the helm-values-command branch 4 times, most recently from 7e172c3 to 340b290 Compare April 12, 2022 12:35

mikesmithgh commented Apr 12, 2022

View reviewed changes

mikesmithgh marked this pull request as ready for review April 12, 2022 12:43

mikesmithgh force-pushed the helm-values-command branch from 340b290 to d34ce3c Compare April 12, 2022 12:45

mikesmithgh changed the title ~~fix: Add dlv to debug dockerfile and expose command in helm chart~~ BREAKING CHANGE: Use commandArgs and add dlv debug helm chart values.yaml Apr 12, 2022

mikesmithgh changed the title ~~BREAKING CHANGE: Use commandArgs and add dlv debug helm chart values.yaml~~ feat(helm): Use commandArgs for all command arguments and add dlv debug helm test Apr 12, 2022

mikesmithgh added 4 commits May 3, 2022 09:27

ci(helm): Add helm chart testing

3bfbcc1

Signed-off-by: Mike Smith <10135646+mjsmith1028@users.noreply.github.com>

test(helm): Add test for leader election

53f580e

Signed-off-by: Mike Smith <10135646+mjsmith1028@users.noreply.github.com>

fix: Add dlv to debug dockerfile and expose command in helm chart

255e331

Signed-off-by: Mike Smith <10135646+mjsmith1028@users.noreply.github.com>

mikesmithgh force-pushed the helm-values-command branch from d34ce3c to 9be8f2b Compare May 3, 2022 13:27

grzesuav approved these changes May 4, 2022

View reviewed changes

grzesuav merged commit b78476e into metacontroller:master May 4, 2022

grzesuav added the released label May 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(helm): Use commandArgs for all command arguments and add dlv debug helm test #484

feat(helm): Use commandArgs for all command arguments and add dlv debug helm test #484

mikesmithgh commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 25, 2022 •

edited

Loading

mikesmithgh Mar 29, 2022

mikesmithgh Apr 12, 2022

grzesuav Mar 30, 2022

mikesmithgh Apr 11, 2022

grzesuav Mar 30, 2022

mikesmithgh Apr 5, 2022

mikesmithgh commented Apr 11, 2022

mikesmithgh Apr 12, 2022

mikesmithgh Apr 12, 2022

mikesmithgh commented Apr 12, 2022

mikesmithgh commented Apr 12, 2022

grzesuav commented May 4, 2022

feat(helm): Use commandArgs for all command arguments and add dlv debug helm test #484

feat(helm): Use commandArgs for all command arguments and add dlv debug helm test #484

Conversation

mikesmithgh commented Mar 25, 2022 • edited Loading

codecov bot commented Mar 25, 2022 • edited Loading

Codecov Report

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

mikesmithgh commented Apr 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikesmithgh commented Apr 12, 2022

mikesmithgh commented Apr 12, 2022

grzesuav commented May 4, 2022

mikesmithgh commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 25, 2022 •

edited

Loading