-
Notifications
You must be signed in to change notification settings - Fork 295
Add RST support in Sandbox #2917
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
Conversation
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getClaimsList_production(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getSmdrList_production(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getClaimsList_sandbox(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getSmdrList_sandbox(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
weiminyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weiminyu resolved 8 discussions.
Reviewable status: 0 of 12 files reviewed, all discussions resolved (waiting on @CydeWeys and @gbrodman).
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbrodman reviewed 12 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @CydeWeys and @weiminyu).
core/src/test/java/google/registry/tmch/RstTmchUtilsTest.java line 39 at r2 (raw file):
try { PRODUCTION.setup(); assertThat(getClaimsList(tld)).isEmpty();
shouldn't we verify that in production we return the default claims list (that we'd have to create in a before method)?
same question for smdrl
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 47 at r2 (raw file):
private static final FluentLogger logger = FluentLogger.forEnclosingClass(); enum RstType {
this seems odd, why do we have to distinguish between these two if we're running all of this in the sandbox (OTE) environment? is that a difference on their end?
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 63 at r2 (raw file):
/** * Returns appropriate test labels if {@code tld} is for RST testing; otherwise returns {@code * defaultList}.
i don't think "defaultList" is actually referenced anywhere as such (same with the method below)
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 90 at r2 (raw file):
} private static Optional<ClaimsList> getClaimList(RstType rstType) {
nit: getClaimsList (with an s)
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 98 at r2 (raw file):
try { return Optional.of(ClaimsListParser.parse(readLines(resource, UTF_8))); } catch (IOException fnfe) {
hah, what's up with the exception variable name here? (same in the next method too)
weiminyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weiminyu made 3 comments and resolved 1 discussion.
Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @gbrodman).
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 47 at r2 (raw file):
Previously, gbrodman wrote…
this seems odd, why do we have to distinguish between these two if we're running all of this in the sandbox (OTE) environment? is that a difference on their end?
Renamed enum class name. These refers to RST's ote vs prod.
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 63 at r2 (raw file):
Previously, gbrodman wrote…
i don't think "defaultList" is actually referenced anywhere as such (same with the method below)
Done.
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 98 at r2 (raw file):
Previously, gbrodman wrote…
hah, what's up with the exception variable name here? (same in the next method too)
Done.
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbrodman reviewed 1 file and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu).
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 47 at r2 (raw file):
Previously, weiminyu (Weimin Yu) wrote…
Renamed enum class name. These refers to RST's ote vs prod.
got it, so this is all just entirely on their end thx
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 2 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @weiminyu).
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 100 at r3 (raw file):
// Do not throw. logger.atSevere().withCause(e).log( "Could not load Claims list for %s in Sandbox.", rstEnvironment);
Log the file name / resource path it's attempting to load here too.
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 116 at r3 (raw file):
// Do not throw. logger.atSevere().withCause(e).log( "Could not load Claims list for %s in Sandbox.", rstEnvironment);
Ditto.
Added RST test label files as resources. Added a RstTmchUtils class that loads appropriate labels according to TLD pattern. Temporarily changed label fetching in production to include the TLD string, so that the new class may know which set of labels to use.
weiminyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weiminyu made 4 comments.
Reviewable status: 11 of 13 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @gbrodman).
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 47 at r2 (raw file):
Previously, gbrodman wrote…
got it, so this is all just entirely on their end thx
Resolve
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 100 at r3 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Log the file name / resource path it's attempting to load here too.
Done.
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 116 at r3 (raw file):
Previously, CydeWeys (Ben McIlwain) wrote…
Ditto.
Done.
core/src/test/java/google/registry/tmch/RstTmchUtilsTest.java line 39 at r2 (raw file):
Previously, gbrodman wrote…
shouldn't we verify that in production we return the default claims list (that we'd have to create in a
beforemethod)?same question for smdrl
Added a new test class. See above.
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getClaimsList_production(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getSmdrList_production(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getClaimsList_sandbox(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getSmdrList_sandbox(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
weiminyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weiminyu resolved 4 discussions.
Reviewable status: 11 of 13 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @gbrodman).
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbrodman reviewed 2 files and all commit messages, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys).
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @weiminyu).
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @weiminyu).
Added RST test label files as resources.
Added a RstTmchUtils class that loads appropriate labels according to TLD pattern.
Temporarily changed label fetching in production to include the TLD string, so that the new class may know which set of labels to use.
This change is