Add support for configuration of credential helper with environment variables

[tobiade]

Fixes #2814

[zhumin8]

Thanks for your contribution!
I've triggered test runs and our team will review.

[chanseokoh]

We don't run integration tests on GitHub Actions but on a different CI. And we use Java 8 there.

<source>1.8</source>
<target>1.8</target>

[chanseokoh]

Looks like we don't have an integration test using <credHelper>gcr</credHelper>, which should continue to work. Could you add that too? Thanks!

[chanseokoh]

I think we can just do

Suggested change

	@Parameter private CredHelperParameters credHelper;
	@Parameter private CredHelperParameters credHelper = new CredHelperParameters();

and remove the constructor you added.

[tobiade]

Makes sense - will refactor. Out of curiosity - are there any other benefits to the inline initialization you suggested apart from less code?

[chanseokoh]

In this case, nothing other than that, IMO. But we kinda prefer, e.g.,

class Pool {
    private boolean resize = true;
    private Integer size = 10;
}

to

class Pool {
    private boolean resize;
    private Integer size;

    Pool() {
        this.resize = true;
        this.size = new Integer(10);
    }
}

and have been applying this style consistently.

[tobiade]

Got it

[chanseokoh]

Can be removed by the above.

[chanseokoh]

ditto

Suggested change

	@Parameter private Map<String, String> environment;
	@Parameter private Map<String, String> environment = new HashMap<>();

and remove the constructor.

[chanseokoh]

We are not really a fan of inheritance for this kind of usage. I'd usually add ImageParameters as a field. But then, the parameter structure doesn't match the actual Jib configuration. (That is, AuthParameters should be a field as we have image.auth.) I think it's still fine to add the cred helper parameters as a field. What do you think?

[tobiade]

Hmm I think I get what you mean. I was trying to reduce the code duplication between BaseImageParameters and TargetImageParameters. I guess making ImageParameters a field would mean Jib configuration might have to change? e.g to configure auth we might need to do something like from.image.auth as opposed to from.auth?

If that's the case, I'm happy to revert and keep as it was before while making CredHelperParameters fields in both
BaseImageParameters and TargetImageParameters.

[chanseokoh]

I guess making ImageParameters a field would mean Jib configuration might have to change? e.g to configure auth we might need to do something like from.image.auth as opposed to from.auth?

I think technically it is not impossible to have ImageParameters as a field while retaining the current Jib configuration structure. However, I don't really like doing that. I'd rather have the code structure in sync with Jib configuration.

So yeah, let's keep CredHelperParameters as a field.

[chanseokoh]

I've triggered test runs and our team will review.

@zhumin8 just FYI, for external contributions, we need to add the kokoro:run label to trigger Kokoro.

[zhumin8]

@chanseokoh Thanks for the reminder.

[tobiade]

Thanks @chanseokoh. I've addressed comments in latest commit.

[chanseokoh]

Thanks! I'll take another look later.

[chanseokoh]

Overall, looks go to me. Just a few minor comments.

Also, could you update jib-maven-plugin/CHANGELOG.md and jib-gradle-plugin/CHANGELOG.md? (BTW, don't update other docs about the new configuration parameters in this PR, since we update them after making a release.)

[chanseokoh]

And you can ignore the SonarCloud failure.

[tobiade]

Thanks @chanseokoh. Updated as discussed.

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

85.9% Coverage
3.6% Duplication

[chanseokoh]

Awesome! Thanks for your contribution.