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

Tech preview for helm feature #190

Merged
merged 9 commits into from
Aug 28, 2019
Merged

Tech preview for helm feature #190

merged 9 commits into from
Aug 28, 2019

Conversation

nwmac
Copy link
Member

@nwmac nwmac commented Aug 22, 2019

No description provided.

@nwmac nwmac self-assigned this Aug 22, 2019
@nwmac nwmac requested a review from richard-cox August 22, 2019 20:58
@nwmac nwmac added the ready for review Ready for review label Aug 22, 2019
@codecov-io
Copy link

codecov-io commented Aug 22, 2019

Codecov Report

Merging #190 into v2-master will decrease coverage by 0.19%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##           v2-master     #190     +/-   ##
============================================
- Coverage      53.27%   53.08%   -0.2%     
============================================
  Files            914      916      +2     
  Lines          25105    25224    +119     
  Branches        4315     4321      +6     
============================================
+ Hits           13375    13390     +15     
- Misses         11730    11834    +104

@richard-cox richard-cox changed the base branch from v2-master to 1.0-release August 23, 2019 09:28
@richard-cox richard-cox changed the base branch from 1.0-release to v2-master August 23, 2019 09:29
Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

Looks good, spotted the following issues though

  • kube dashboard requires sameorigin fix as per Complete Kube dashboard feature #151
  • helm chart summary add repo/install buttons are invisible, only install command (semi) copies text
  • helm endpoint card is greyed out as if it were disconnected, even when synchronised
  • helm endpoint card 'synchronising' spinner continues after sync has finished. It looks like there's no http request related to helm, could be another status related issue

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

LGTM, will merge once passed gates
One thing to note, with a helm endpoint already registered and helm disabled I got a backend panic in info.go line 97 s.Endpoints[cnsiType][cnsi.GUID] = endpoint. I don't think this is an issue we need to address now though due to the likelihood of an existing helm endpoint or toggling between enabled/disabled.

@richard-cox richard-cox added in review and removed needs attention Needs attention labels Aug 28, 2019
@richard-cox richard-cox merged commit 4ef4f85 into v2-master Aug 28, 2019
@richard-cox richard-cox deleted the suse-tech-preview branch August 28, 2019 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants