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

marketplace: cf endpoint on card when multiple cf connected #3560

Merged
merged 1 commit into from
May 13, 2019

Conversation

vitoravelino
Copy link
Contributor

@vitoravelino vitoravelino commented May 2, 2019

When multiple CF were connected the user could end up with multiple services
with the same name without being able to identify the source of that.

Now when multiple CF endpoints are connected a CF endpoint is shown on
each service card so the user can identify where it comes from.

Signed-off-by: Vítor Avelino vavelino@suse.com

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Docs update
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshot

Screenshot-20190502164119-658x390

Fixes #3026

@cfdreddbot
Copy link

✅ Hey vitoravelino! The commit authors and yourself have already signed the CLA.

@vitoravelino vitoravelino force-pushed the show-cf-marketplace branch from 571dc55 to e82b75c Compare May 2, 2019 20:28
@cloudfoundry cloudfoundry deleted a comment from codecov bot May 2, 2019
@KlapTrap KlapTrap added needs attention This PR needs attention and removed ready for review labels May 7, 2019
@KlapTrap
Copy link
Contributor

KlapTrap commented May 7, 2019

@vitoravelino can you take a look at the FE unit tests. Thanks!

@vitoravelino vitoravelino force-pushed the show-cf-marketplace branch from e82b75c to 76e2952 Compare May 7, 2019 11:27
@cloudfoundry cloudfoundry deleted a comment from codecov bot May 7, 2019
@vitoravelino vitoravelino force-pushed the show-cf-marketplace branch 2 times, most recently from ad28da5 to d071097 Compare May 7, 2019 11:54
@vitoravelino
Copy link
Contributor Author

@vitoravelino can you take a look at the FE unit tests. Thanks!

Fixed.

@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #3560 into v2-master will increase coverage by <.01%.
The diff coverage is 100%.

@@              Coverage Diff              @@
##           v2-master    #3560      +/-   ##
=============================================
+ Coverage      51.83%   51.83%   +<.01%     
=============================================
  Files            720      720              
  Lines          20221    20224       +3     
  Branches        3612     3613       +1     
=============================================
+ Hits           10481    10484       +3     
  Misses          9740     9740

@richard-cox richard-cox added ready for review and removed needs attention This PR needs attention labels May 8, 2019
Copy link
Contributor

@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.

One minor point about import sorting, otherwise LGTM

@@ -5,45 +5,40 @@ import { TabNavService } from '../../../../../tab-nav.service';
import { BaseTestModulesNoShared } from '../../../../../test-framework/cloud-foundry-endpoint-service.helper';
import { ServicesService } from '../../../../features/service-catalog/services.service';
import { ServicesServiceMock } from '../../../../features/service-catalog/services.service.mock';
import { AppNameUniqueDirective } from '../../../app-name-unique.directive/app-name-unique.directive';
Copy link
Contributor

Choose a reason for hiding this comment

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

The sorting of these imports seems different to the one's I'm seeing. Do you use vs code, and if so do you use the TypeScript Hero plugin or have any custom rules? We've probably got something wrong in our docs here either way so would be good to get to the bottom of

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the imports and re-added them automatically. That's probably why it got messy like that unless there's a specific configuration for the typescript hero extension.

@richard-cox richard-cox added needs attention This PR needs attention and removed ready for review labels May 8, 2019
When multiple CF were connected the user could end up with multiple services
with the same name without being able to identify the source of that.

Now when multiple CF endpoints are connected a CF endpoint is shown on
each service card so the user can identify where it comes from.

Signed-off-by: Vítor Avelino <vavelino@suse.com>
@vitoravelino vitoravelino force-pushed the show-cf-marketplace branch from d071097 to c9e8ff6 Compare May 10, 2019 09:45
@richard-cox richard-cox added ready for review and removed needs attention This PR needs attention labels May 10, 2019
Copy link
Contributor

@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, once passed gates this is good to merge

@richard-cox richard-cox merged commit 4a4b487 into v2-master May 13, 2019
@richard-cox richard-cox deleted the show-cf-marketplace branch May 13, 2019 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Marketplace: When multiple CFs connected show service CF in card
4 participants