Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement GKE ManagedCertificate CRD health checks #3600

Merged

Conversation

micke
Copy link
Contributor

@micke micke commented May 16, 2020

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Optional. My organization is added to USERS.md.
  • I've signed the CLA and my build is green (troubleshooting builds).

@CLAassistant
Copy link

CLAassistant commented May 16, 2020

CLA assistant check
All committers have signed the CLA.

@micke micke changed the title Implement GKE ManagedCertificate CRD health checks feat: Implement GKE ManagedCertificate CRD health checks May 16, 2020
@micke micke marked this pull request as ready for review May 17, 2020 10:17
@micke
Copy link
Contributor Author

micke commented May 17, 2020

I'm having some issues setting up my local environment, i'm following https://argoproj.github.io/argo-cd/developer-guide/contributing

I will continue to try and make it work but i figured that health checks tests run in the CI too?

@jannfis
Copy link
Member

jannfis commented May 17, 2020

Hi @micke, thanks for your contribution! If you need any support with setting up your toolchain, feel free to ask anything and also to suggest improvements to the documentation!

As for your question, yes, the health checks will run in the CI too (or when running make test in your toolchain). I'm not sure why the CI on this PR is in pending state, I can only imagine that CircleCI was having issues at the time you submitted the PR. Please try to retrigger it according to https://argoproj.github.io/argo-cd/developer-guide/faq/#can-i-retrigger-the-checks-without-pushing-a-new-commit

@micke
Copy link
Contributor Author

micke commented May 17, 2020

@jannfis Thanks for the quick response!
Thanks, will do!

I triggered another build and it looks like it's running now! Cheers!

@codecov
Copy link

codecov bot commented May 17, 2020

Codecov Report

Merging #3600 into master will increase coverage by 2.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3600      +/-   ##
==========================================
+ Coverage   39.09%   41.28%   +2.19%     
==========================================
  Files          49      114      +65     
  Lines        1407    16445   +15038     
  Branches      238        0     -238     
==========================================
+ Hits          550     6790    +6240     
- Misses        854     8774    +7920     
- Partials        3      881     +878     
Impacted Files Coverage Δ
.../shared/components/tags-input/tags-input-field.tsx
ui/src/app/shared/services/applications-service.ts
ui/src/app/shared/services/repocreds-service.ts
ui/src/app/shared/components/spinner.tsx
.../src/app/applications/components/resource-label.ts
ui/src/app/shared/models.ts
...rc/app/shared/services/view-preferences-service.ts
...mponents/application-parameters/kustomize-image.ts
.../app/shared/components/yaml-editor/yaml-editor.tsx
...hared/components/editable-panel/editable-panel.tsx
... and 153 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 c21a6ea...4c9a1ff. Read the comment docs.

@micke
Copy link
Contributor Author

micke commented May 17, 2020

I'm not sure but i think the last failing check is something i can't really do anything about.

So i guess this is ready for review!

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot @micke

@jannfis jannfis merged commit 991ee9b into argoproj:master May 17, 2020
@micke micke deleted the implement-gke-managed-certificate-health-checks branch May 17, 2020 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants