Skip to content

Rewrite JWTProxyConfigBuilder to use model objects to create config YAML#10633

Merged
mshaposhnik merged 12 commits intomasterfrom
che-10402
Aug 3, 2018
Merged

Rewrite JWTProxyConfigBuilder to use model objects to create config YAML#10633
mshaposhnik merged 12 commits intomasterfrom
che-10402

Conversation

@mkuznyetsov
Copy link
Copy Markdown
Contributor

What does this PR do?

Add new objects for JWT Proxy config, and use ObjectMapper to generate YAML instead of formattign String template

What issues does this PR fix or reference?

#10402

@mkuznyetsov mkuznyetsov requested a review from garagatyi as a code owner August 2, 2018 07:18
@mkuznyetsov mkuznyetsov changed the title [WIP] Rewrite JWTProxyConfigBuilder to use model objects to create yaml [WIP] Rewrite JWTProxyConfigBuilder to use model objects to create config YAML Aug 2, 2018
@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Aug 2, 2018
@mshaposhnik mshaposhnik changed the title [WIP] Rewrite JWTProxyConfigBuilder to use model objects to create config YAML Rewrite JWTProxyConfigBuilder to use model objects to create config YAML Aug 2, 2018
@mshaposhnik
Copy link
Copy Markdown
Contributor

ci-test

@riuvshin
Copy link
Copy Markdown
Contributor

riuvshin commented Aug 2, 2018

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10633
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@mshaposhnik
Copy link
Copy Markdown
Contributor

ci-test

@riuvshin
Copy link
Copy Markdown
Contributor

riuvshin commented Aug 2, 2018

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10633
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@mshaposhnik
Copy link
Copy Markdown
Contributor

ci-test

@riuvshin
Copy link
Copy Markdown
Contributor

riuvshin commented Aug 3, 2018

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10633
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@mshaposhnik mshaposhnik requested a review from skabashnyuk August 3, 2018 12:55
@mshaposhnik mshaposhnik merged commit daa4441 into master Aug 3, 2018
@mshaposhnik mshaposhnik deleted the che-10402 branch August 3, 2018 13:30
private List<VerifierProxyConfig> verifiedProxyConfigs;

@JsonProperty("signer_proxy")
private SignerProxy signerProxy;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be better to use the same style of naming for VerifierProxyConfig and SignerProxy. Both with or without suffix Config

try {
return YAML_PARSER.writeValueAsString(config);
} catch (JsonProcessingException e) {
throw new RuntimeException("Error during creation of JWTProxy config YAML: ", e);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be better to use InfrastructureException or even InternalInfrastructureException instead of RuntimeException because I'm not sure that RuntimeException will be handled correctly.
Also, I guess "Error during creation of JWTProxy config YAML: " is incomplete and e.getMessage should be added there.


@JsonInclude(Include.NON_NULL)
/**
* Desribes JWT verifier configuration
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo in word Desribes

*
* @author Mykhailo Kuznietsov
*/
public class Config {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Config is a quite common object name. Maybe it would be better to add JwtProxy specific prefix

@benoitf benoitf added this to the 6.10.0 milestone Aug 9, 2018
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/task Internal things, technical debt, and to-do tasks to be performed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants