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

[Misc] Add Mozilla cacerts #28

Merged
merged 1 commit into from
May 23, 2019
Merged

Conversation

yfxhust
Copy link
Contributor

@yfxhust yfxhust commented May 17, 2019

Summary: Add mozilla cacerts

Test Plan: Covered by Mozilla Network Security Service

Reviewed-by: D-D-H, luchsh

Issue: #29

@CLAassistant
Copy link

CLAassistant commented May 17, 2019

CLA assistant check
All committers have signed the CLA.

@luchsh luchsh requested review from luchsh, D-D-H and kuaiwei May 17, 2019 08:11
@luchsh
Copy link
Contributor

luchsh commented May 17, 2019

1, pls sign CLA, anyway
2, let's move this file to some more suitable directory,
not the root directory :(

@yfxhust yfxhust force-pushed the cacerts branch 2 times, most recently from 2b991df to d6570a2 Compare May 17, 2019 08:55
@yfxhust yfxhust changed the title [Misc] Add mozilla cacerts [Misc] Add Mozilla cacerts May 17, 2019
@yfxhust
Copy link
Contributor Author

yfxhust commented May 17, 2019

1, pls sign CLA, anyway
2, let's move this file to some more suitable directory,
not the root directory :(

Accept. Already move cacerts file to common/security. I also add another cacerts.list.txt to make cacerts human readable.

@luchsh
Copy link
Contributor

luchsh commented May 17, 2019

Please create an issue, link it to this patch, and update the 'Issue: ' field from commit template.

@luchsh
Copy link
Contributor

luchsh commented May 17, 2019

Test Plan: Covered by Mozilla Network Security Service

That does sound sufficient to me, is there any existing JTreg/JCK tests related to this change?

@luchsh
Copy link
Contributor

luchsh commented May 17, 2019

1, pls sign CLA, anyway
2, let's move this file to some more suitable directory,
not the root directory :(

Accept. Already move cacerts file to common/security. I also add another cacerts.list.txt to make cacerts human readable.

how was cacerts.list.txt created? is it just for human to read?
pls add copyright header to that file if possible.

@yfxhust
Copy link
Contributor Author

yfxhust commented May 22, 2019

Make some improvement according to luchsh comments.

  1. add Isssue field in comment
  2. add copyright in cacerts.list.txt
  3. This cacerts already verified by Mozilla nss project. Introducing nss test is not neccessary.

@yfxhust yfxhust removed the request for review from kuaiwei May 22, 2019 08:50
@D-D-H
Copy link
Collaborator

D-D-H commented May 22, 2019

             --with-cacerts-file=`pwd`/common/security/cacerts \

pwd is not robust, how about readlink -f ./

Summary: Add Mozilla cacerts

Test Plan: Covered by Mozilla Network Security Service

Reviewed-by: D-D-H, luchsh

Issue: dragonwell-project#29
@D-D-H D-D-H assigned D-D-H and yfxhust and unassigned yfxhust and D-D-H May 23, 2019
Copy link
Collaborator

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

@yfxhust yfxhust merged commit 90cf1d1 into dragonwell-project:master May 23, 2019
@@ -62,12 +60,17 @@ else
BUILD_INDEX=b$BUILD_NUMBER
fi

DISTRO_VERSION=${DRAGONWELL_VERSION}-${BUILD_INDEX}
DISTRO_NAME="Alibaba Dragonwell"

Copy link
Contributor

Choose a reason for hiding this comment

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

pls move this change to another patch。
and you should update common/autoconf/spec.gmk.in instead

DRAGONWELL_JDK_UPDATE_VERSION=202
DRAGONWELL_JDK_BUILD_NUMBER=b01
Copy link
Contributor

Choose a reason for hiding this comment

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

should not change this, pls create another patch

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.

4 participants