Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-551. Add a test of user identity verification #337

Closed
wants to merge 3 commits into from

Conversation

Eroschang
Copy link
Contributor

What is this PR for?

To test the LDAP for user verification.
The user in this open LDAP server can be verified successfully.

What type of PR is it?

Improvement

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/SUBMARINE-551

How should this be tested?

Screenshots (if appropriate)

image

Questions:

  • Does the licenses files need update? /No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

@Eroschang Please add SUBMAINR-ID. xxx to PR title head.


try {
ctx = new InitialDirContext(HashEnv);
System.out.println("Pass");
Copy link
Member

Choose a reason for hiding this comment

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

Please change System.out.println("Pass"); to LOG.info(...);

}
catch (AuthenticationException e) {
System.out.println("fail");
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Please change

e.printStackTrace();

to

LOG.error(e.getMessage(), e);

}
catch (javax.naming.CommunicationException e) {
System.out.println("Connection fail");
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Please change

e.printStackTrace();

to

LOG.error(e.getMessage(), e);

Comment on lines 167 to 168
System.out.println("Unknown identity verification fail");
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Please change

e.printStackTrace();

to

LOG.error(e.getMessage(), e);

@Eroschang Eroschang changed the title Add a test of user identity verification. SUBMARINE-551. Add a test of user identity verification. Jul 1, 2020
Copy link
Member

@jiwq jiwq left a comment

Choose a reason for hiding this comment

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

@@ -133,6 +136,39 @@ public void testList() throws Exception {
context.close();
}

@Test
public void testauth() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public void testauth() throws Exception {
public void testAuth() throws Exception {

LOG.info("Pass");
}
catch (AuthenticationException e) {
LOG.info("fail");
Copy link
Member

Choose a reason for hiding this comment

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

Please delete LOG.info(fail)
Because there is already have LOG.error(e.getMessage(), e);

LOG.error(e.getMessage(), e);
}
catch (javax.naming.CommunicationException e) {
LOG.info("Connection fail");
Copy link
Member

Choose a reason for hiding this comment

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

Please delete LOG.info("Connection fail");
Because there is already have LOG.error(e.getMessage(), e);

LOG.error(e.getMessage(), e);
}
catch (Exception e) {
LOG.info("Unknown identity verification fail");
Copy link
Member

Choose a reason for hiding this comment

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

Please delete LOG.info("Unknown identity verification fail");
Because there is already have LOG.error(e.getMessage(), e);

@xunliu xunliu changed the title SUBMARINE-551. Add a test of user identity verification. SUBMARINE-551. Add a test of user identity verification Jul 2, 2020
Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

LGMT
Will merge if no more comments.

@asfgit asfgit closed this in b85d409 Jul 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants