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

[NCL-4538] Add new BUILDER_POD_MEMORY parameter #2376

Merged
merged 1 commit into from Apr 9, 2019

Conversation

janinko
Copy link
Contributor

@janinko janinko commented Apr 8, 2019

Checklist:

  • Have you added a note in the CHANGELOG.md for your change if user-facing?
  • Have you added unit tests for your change?

@janinko
Copy link
Contributor Author

janinko commented Apr 8, 2019

Waiting for 1.5 branch for this PR

@janinko janinko requested a review from matejonnet April 8, 2019 13:44
@@ -48,7 +51,8 @@ StartedEnvironment startEnvironment(
SystemImageType systemImageType,
RepositorySession repositorySession,
DebugData debugData,
String accessToken) throws EnvironmentDriverException;
String accessToken,
Map<String, String> parameters) throws EnvironmentDriverException;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to use specific parameter instead of the map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand (generic) parameters to be a map that is passed to various services and they rocognize their parameters and use them: CUSTOM_PME_PARAMETERS is consumed by PME, EXECUTION_ROOT_NAME is consumed by Repour and BUILDER_POD_MEMORY would be consumed by environment drivers.

You think it should be some other service/part that would consume the BUILDER_POD_MEMORY parameter?

Copy link
Contributor

Choose a reason for hiding this comment

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

The API that defines what is required rather than taking the map of Strings is more readable.
The other thing is OpenshiftStartedEnvironment it is an implementation details that does not need to know about the Keys in the generic parametrs map.
I see the use case you described more on the REST api level. On the other side I see that repository driver has been done in the same way.

@jbartece
Copy link
Contributor

jbartece commented Apr 9, 2019

@janinko Don't wait for branch 1.5. There are also commits in the 1.4, which will be released as 1.5. Once the PR is ready, merge it. We will sort out the branches when releasing.

@jbartece jbartece closed this Apr 9, 2019
@jbartece
Copy link
Contributor

jbartece commented Apr 9, 2019

Sorry, missclick. Why are the buttons to close and comment so close to each other...

@jbartece jbartece reopened this Apr 9, 2019
@janinko janinko marked this pull request as ready for review April 9, 2019 13:36
@janinko janinko dismissed matejonnet’s stale review April 9, 2019 15:54

we agreed to keep it as is

@janinko janinko merged commit cfa408d into project-ncl:v1.4 Apr 9, 2019
@janinko janinko deleted the NCL-4538 branch April 30, 2019 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants