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

Improve usability of authentication client #349

Merged
merged 39 commits into from
Aug 14, 2019

Conversation

ilkinabdullayev
Copy link
Contributor

@ilkinabdullayev ilkinabdullayev commented Aug 9, 2019

Happy Reviews!

@codecov
Copy link

codecov bot commented Aug 9, 2019

Codecov Report

Merging #349 into master will increase coverage by 3.34%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #349      +/-   ##
============================================
+ Coverage     66.69%   70.04%   +3.34%     
  Complexity       12       12              
============================================
  Files           216      234      +18     
  Lines          4138     4353     +215     
  Branches        547      560      +13     
============================================
+ Hits           2760     3049     +289     
+ Misses         1239     1159      -80     
- Partials        139      145       +6
Impacted Files Coverage Δ Complexity Δ
...ty/common/error/ServiceNotAccessibleException.java 100% <ø> (ø) 0 <0> (?)
...y/common/error/ResourceAccessExceptionHandler.java 100% <ø> (ø) 0 <0> (?)
...m/ca/mfaas/gateway/security/query/QueryFilter.java 70.58% <ø> (ø) 0 <0> (ø) ⬇️
...ay/security/query/TokenAuthenticationProvider.java 100% <ø> (ø) 0 <0> (ø) ⬇️
...ml/security/common/content/BasicContentFilter.java 100% <ø> (ø) 0 <0> (?)
...iml/security/common/token/TokenAuthentication.java 100% <ø> (ø) 0 <0> (?)
...com/ca/mfaas/apicatalog/ApiCatalogApplication.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...faas/product/gateway/GatewayNotFoundException.java 100% <ø> (ø) 0 <0> (ø) ⬇️
...y/common/handler/BasicAuthUnauthorizedHandler.java 100% <ø> (ø) 0 <0> (?)
...curity/common/token/TokenNotProvidedException.java 100% <ø> (ø) 0 <0> (?)
... and 105 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 b3972d3...c5d06af. Read the comment docs.

jandadav and others added 8 commits August 9, 2019 14:19
Signed-off-by: janda06 <david.janda@broadcom.com>
changed catalog to use the lookup service
various fixes for catalog startup
aligned componentscan to common place for gw

Signed-off-by: janda06 <david.janda@broadcom.com>
Signed-off-by: janda06 <david.janda@broadcom.com>
- Deleted gateway login url
- Removed @EnableConfigurationProperties from main classes
- Removed @configuration from main classes
- Some refactoring
- Unit tests disabled temporary
Signed-off-by: janda06 <david.janda@broadcom.com>
Signed-off-by: at670475 <andrea.tabone@broadcom.com>
Signed-off-by: janda06 <david.janda@broadcom.com>
Copy link
Contributor

@JirkaAichler JirkaAichler left a comment

Choose a reason for hiding this comment

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

Appreciate the hard work. Thank you!

Copy link
Contributor

@arxioly arxioly left a comment

Choose a reason for hiding this comment

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

Well done! Thanks!

@ilkinabdullayev ilkinabdullayev merged commit d66d297 into master Aug 14, 2019
@delete-merged-branch delete-merged-branch bot deleted the janda06/SSCGatewayConfigRework branch August 14, 2019 14:37
@@ -104,7 +106,7 @@ private void processInstance(Applications filteredServices, Application applicat
String value = instanceInfo.getMetadata().get(API_ENABLED_METADATA_KEY);
boolean apiEnabled = true;
if (value != null) {
apiEnabled = Boolean.valueOf(value);
apiEnabled = Boolean.parseBoolean(value);
}

// only add api enabled services
Copy link
Contributor

Choose a reason for hiding this comment

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

Only add? or add only API enabled services?

@@ -42,11 +45,17 @@

/**
* Periodically refresh the container/service caches
* Depends on the GatewayClient: no refreshes happen when it's not initialized
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather explain what will happen if "it's initiated". For example "The GatewayClient initialization refreshes the container/service cache"

@@ -149,16 +158,16 @@ private void processServiceInstance(Set<String> containersUpdated, Applications
}

/**
* Go ahead amd retrieve this instances API doc and update the cache
* Go ahead and retrieve this instances API doc and update the cache
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make much sense grammatically.
"Ensure that you retrieve the API doc of this instance, and update the cache". Also we need to make sure it's understood what instance we refer to here. What is "This instance"?

import java.net.URI;

/**
* GatewayInstanceInitializer takes care about starting the lookup for Gateway instance after the context is started
Copy link
Contributor

Choose a reason for hiding this comment

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

"GatewayInstanceInitializer starts the lookup for Gateway instance after the context is started"
"takes care" is too vague


/**
* GatewayInstanceInitializer takes care about starting the lookup for Gateway instance after the context is started
* Its meant to be created as a bean, as it is for example by SecurityServiceConfiguration in security-service-client-spring
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rewrite this - has grammatical mistakes and is generally hard to understand

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.

8 participants