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.

Sign up for GitHub

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

Jump to bottom

Change jibSerialize to jib.serialize #1968

Merged
merged 2 commits into from
Sep 10, 2019
Merged

Change jibSerialize to jib.serialize #1968

merged 2 commits into from
Sep 10, 2019

Conversation

loosebazooka
Copy link
Member

part of #1967, take a look at where the warning is happening, open to suggestions on other ways to do this.

briandealwis
briandealwis reviewed Sep 10, 2019
View reviewed changes
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

IMHO we shouldn't bother with the deprecation notice: it's a debugging property, not a production property. We allow seeing the executor which is what someone should do if they want control.

chanseokoh
chanseokoh reviewed Sep 10, 2019
View reviewed changes
Copy link
Member

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

I also think it is OK to just deprecate the old one. I can't think of any reason anyone actually uses the flag in practice.

docs/faq.md Outdated
@@ -533,16 +533,15 @@ com.google.api.client.http.level=CONFIG

And then launch your build tool as follows:
```sh
mvn -Djava.util.logging.config.file=path/to/log.properties -DjibSerialize=true -Djib.console=plain ...
mvn -Djava.util.logging.config.file=path/to/log.properties -Djib.serialize=true ...
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to retain -Djib.console=plain. Almost a requirement when redirecting long output to a file.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh oops, must've been lost in a rebase

jib-core/src/test/java/com/google/cloud/tools/jib/api/JibContainerBuilderTest.java Outdated
@@ -49,13 +52,16 @@
@RunWith(MockitoJUnitRunner.class)
public class JibContainerBuilderTest {

@Rule public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
Copy link
Member

Choose a reason for hiding this comment

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

final

@TadCordle
Copy link
Contributor

Should we add a jib-core changelog entry too?

@chanseokoh
Copy link
Member

I think we should hold off merging this until right before we make the next release, since it updates the FAQ to use the new (not yet live) flag.

@loosebazooka
Copy link
Member Author

IMHO we shouldn't bother with the deprecation notice: it's a debugging property, not a production property. We allow seeing the executor which is what someone should do if they want control.

I think this is harder when we have to change the property sendCredentialsOverHttp to jib.sendCredentialsOverHttp, also I'm a little confused why these properties are separate from jib-plugins-common/.../PropertyNames.java

@loosebazooka
Copy link
Member Author

loosebazooka commented Sep 10, 2019 •
edited
Loading

I think we should hold off merging this until right before we make the next release, since it updates the FAQ to use the new (not yet live) flag.

I can move the faq changes into the 1.6 PR: #1938 a new docs pr for this change #1973

@loosebazooka loosebazooka changed the title Change jibSerialize to jib.serialize with deprecation warning Change jibSerialize to jib.serialize Sep 10, 2019
@chanseokoh
Copy link
Member

I think this is harder when we have to change the property sendCredentialsOverHttp to jib.sendCredentialsOverHttp, also I'm a little confused why these properties are separate from jib-plugins-common/.../PropertyNames.java

Things in PropertyNames can apply only to our plugins. For example, jib.console makes sense only with the build loggers.

@loosebazooka loosebazooka merged commit 85bb1c5 into master Sep 10, 2019
@loosebazooka loosebazooka deleted the jibSerialize branch September 10, 2019 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chanseokoh chanseokoh chanseokoh left review comments

@briandealwis briandealwis briandealwis left review comments

@TadCordle TadCordle TadCordle approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@loosebazooka @TadCordle @chanseokoh @briandealwis @googlebot