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

Support Compose memory limit byte units #8837

Merged
merged 6 commits into from
Feb 20, 2018
Merged

Support Compose memory limit byte units #8837

merged 6 commits into from
Feb 20, 2018

Conversation

mkuznyetsov
Copy link
Contributor

What does this PR do?

Allow using byte units for defining memory limit in Compose configuration (numbers followed my "b, kb", etc.)

What issues does this PR fix or reference?

#8232

@mkuznyetsov mkuznyetsov added the kind/bug Outline of a bug - must adhere to the bug report template. label Feb 19, 2018
@mkuznyetsov mkuznyetsov self-assigned this Feb 19, 2018
import com.fasterxml.jackson.databind.JsonDeserializer;
import java.io.IOException;
import org.eclipse.che.commons.lang.Size;

Copy link
Contributor

Choose a reason for hiding this comment

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

javadoc

@mkuznyetsov
Copy link
Contributor Author

ci-test

@codenvy-ci
Copy link

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

Copy link

@garagatyi garagatyi 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.
I also added inlined comments how to improve the PR, but they are not blocking.

try {
return Size.parseSize((String) memLimit);
} catch (IllegalArgumentException e) {
throw ctxt.mappingException(format(UNSUPPORTED_VALUE_MESSAGE, memLimit));

Choose a reason for hiding this comment

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

You can ignore this exception since the following operation throws the same exception


private ComposeEnvironmentFactory factory;

private static final String RECIPE_WITHOUT_COMMAND_VALUE =

Choose a reason for hiding this comment

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

This test is not related to a command. Looks like the name is a copy-paste error. BTW can we remove everything not related to these tests from the recipe to make class clearer?

@Test(dataProvider = "validValues")
public void shouldDeserializeValues(Object memoryLimit, Object parsedLimit)
throws ValidationException {
ComposeRecipe service =

Choose a reason for hiding this comment

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

Name of var is misleading - it is a recipe that may contain a number of services.

public Object[][] validValues() {
return new Object[][] {
{"2048", 2048L},
{"1gb", 1073741824L},

Choose a reason for hiding this comment

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

Can you replace these magic constants with something evaluated from constants?
E.g.

private static final long K = 1024;
...
private static final long G = 1024 * M;

{"1gb", 1 * G},

@mkuznyetsov mkuznyetsov merged commit 1904eac into master Feb 20, 2018
@mkuznyetsov mkuznyetsov deleted the che-8232 branch February 20, 2018 08:54
@benoitf benoitf added this to the 6.2.0 milestone Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants