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

Forbid more characters from request/deploy IDs #1407

Merged
merged 3 commits into from Feb 10, 2017

Conversation

@PtrTeixeira
Copy link
Contributor

PtrTeixeira commented Jan 26, 2017

In an effort to prevent attempting to create docker containers
with names that docker will refuse, this forbids more characters
from being used in request/deploy IDs. This is actually more of a
whitelist approach than a blacklist approach; it explicitly
says the characters that can be used and forbids anything else.

Closes #1406
/cc @ssalinas

In an effort to prevent attempting to create docker containers
with names that docker will refuse, this forbids more characters
from being used in request/deploy IDs. This is actually more of a
whitelist approach than a blacklist approach; it explicitly
says the characters that can be used and forbids anything else.
@@ -0,0 +1,63 @@
package com.hubspot.singularity.data;

import static org.mockito.Mockito.*;

This comment has been minimized.

@ssalinas

ssalinas Jan 27, 2017 Member

We generally try to stay away from any import * if we can. Can probably tune your IDE settings for this

@Rule
public ExpectedException expectedException = ExpectedException.none();

public static class TestModule extends JukitoModule {

This comment has been minimized.

@ssalinas

ssalinas Jan 27, 2017 Member

We actually have a class set up that handles most of this setup already. If you just add these tests in the ValidatorTest class (feel free to rename to SingularityValidatorTest for consistency), that class already has all of the guice bits taken care of and you can use the already injected SingularityValidator. Saves us from adding the mockito dep for just this class as well ;)

Move the code from the validator test class that I had created into
a the existing validator test class. This let me get rid of the
dependencies that I had added as a part of testing my code.
public void itForbidsBracketCharactersInDeployIds() throws Exception {
final String badDeployId = "deployKey[[";

SingularityDeploy singularityDeploy = new SingularityDeploy(badDeployId, badDeployId,

This comment has been minimized.

@ssalinas

ssalinas Jan 31, 2017 Member

There are SingularityDeployBuilder and SingularityRequestBuilder methods for this so that you don't need to use the giant constructor. For example you could do:
SingularityDeploy.newBuilder("requestId", "deployId")

Fix that up and I think this PR is all set 👍

Use builders to create the Singularity{Request,Deploy} instances that
were needed for testing. This lets the contructors and all of their
`Optional.<>absent()` arguments be elided, which makes the whole think
much, much easier to read. Thanks to Stephen for the fix.
@ssalinas ssalinas modified the milestone: 0.14.0 Feb 9, 2017
@ssalinas ssalinas added the hs_stable label Feb 10, 2017
@ssalinas
Copy link
Member

ssalinas commented Feb 10, 2017

LGTM

@ssalinas ssalinas merged commit 9c71343 into master Feb 10, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ssalinas ssalinas deleted the expand-deployId-character-blacklist branch Feb 10, 2017
@ssalinas ssalinas removed hs_qa labels Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.