You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by William Chu <no...@github.com> on 2014/12/03 00:10:35 UTC
[jclouds] JCLOUDS-788 Added new method to support the passing of a
custom node nam... (#622)
...e when generating the chef bootstrap node script
You can merge this Pull Request by running:
git pull https://github.com/bertramdev/jclouds master
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/622
-- Commit Summary --
* JCLOUDS-788 Added new method to support the passing of a custom node name when generating the chef bootstrap node script
-- File Changes --
M apis/chef/src/main/java/org/jclouds/chef/functions/GroupToBootScript.java (41)
M apis/chef/src/test/java/org/jclouds/chef/functions/GroupToBootScriptTest.java (20)
A apis/chef/src/test/resources/bootstrap-node-env.sh (56)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/622.patch
https://github.com/jclouds/jclouds/pull/622.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1982](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1982/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65520020
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -175,6 +175,10 @@ public Statement createBootstrapScriptForGroup(String group) {
> return groupToBootScript.apply(group);
> }
>
> + public Statement createBootstrapScriptForGroupAndNodeName(String group, String nodeName) {
Add `@Override` annotation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21201828
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> - *
> - * @param group The group where the given bootstrap configuration will be
> - * applied.
> - * @param bootstrapConfig The configuration to be applied to the nodes in the
> - * group.
> - */
> + Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName);
> +
> + /**
> + * Configures how the nodes of a certain group will be bootstrapped
> + *
> + * @param group The group where the given bootstrap configuration will be
> + * applied.
> + * @param bootstrapConfig The configuration to be applied to the nodes in the
> + * group.
> + */
Can you restore the indent o the javadoc?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21267367
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1980](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1980/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65481807
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -127,4 +127,45 @@ public Statement apply(String group) {
> createFirstBoot, runChef);
> }
>
> + public Statement apply(String group, String nodeName) {
Instead of providing a new function with most of the code duplicated, I'd simply add the `nodeName` parameter, annotated as `@Nullable`, to the existing one, and remove the `implements Function<String, Statement>` from its declaration. That it not used, as we are injecting directly the `GroupToBootScript` type, so you should be able to remove it without issues.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21201954
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1983](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1983/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65525263
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1986](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1986/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65687388
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1974](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1974/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65329940
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @will-districtnerds! Just a couple small comments, and it is good to go. When addressing them, mind squashing all commits into a single one so we can cleanly merge the changes?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65496285
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -171,8 +172,8 @@ String buildBootstrapConfiguration(BootstrapConfig bootstrapConfig) {
> }
>
> @Override
> - public Statement createBootstrapScriptForGroup(String group) {
> - return groupToBootScript.apply(group);
> + public Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName) {
> + return groupToBootScript.apply(group, null);
Hmmm this is wrong now? There should be two methods, one passing null and the other one passing the nodeName.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21296494
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -171,8 +172,8 @@ String buildBootstrapConfiguration(BootstrapConfig bootstrapConfig) {
> }
>
> @Override
> - public Statement createBootstrapScriptForGroup(String group) {
> - return groupToBootScript.apply(group);
> + public Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName) {
> + return groupToBootScript.apply(group, null);
No. I think I didn't explain properly. There must be two methods. The old and the new one so we don't break existing code.
The previous comment pretended to say this, remarking the fact that ot was that way in some previous revision of this PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21321723
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> - * @param group The group where the given bootstrap configuration will be
> - * applied.
> - * @param bootstrapConfig The configuration to be applied to the nodes in the
> - * group.
> - */
> + /**
> + * Creates all steps necessary to bootstrap the node and allows you to specify a custom value
> + * for the node name.
> + *
> + * @param group corresponds to a configured
> + * {@link ChefProperties#CHEF_BOOTSTRAP_DATABAG} data bag where
> + * run_list and other information are stored.
> + * @param nodeName The name of the node to create.
> + * @return The script used to bootstrap the node.
> + */
> + Statement createBootstrapScriptForGroupAndNodeName(String group, String nodeName);
Prefer method overloading. Name it `createBootstrapScriptForGroup` too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21222890
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -175,6 +175,10 @@ public Statement createBootstrapScriptForGroup(String group) {
> return groupToBootScript.apply(group);
> }
>
> + public Statement createBootstrapScriptForGroupAndNodeName(String group, String nodeName) {
> + return groupToBootScript.apply(group, nodeName);
Just FTR, I've had a look at the docs for the `client.rb` file (which is the one that configures the node being created, and I've only found the *nodeName* attribute to be node-specific. All other attributes seem to make sense being "global to a group".
If it turns that other attributes are relevant only to a single node, then the signature should be changed to accept a `NodeOptions` or similar class instead of the String. For the moment, I think it is ok as-is.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21202076
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for the patch @will-districtnerds!
I've had a quick look and left a couple comments.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65327433
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1975](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1975/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65335871
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by William Chu <no...@github.com>.
Thanks for the quick review! Closing until I address all concerns
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65327548
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -175,6 +175,10 @@ public Statement createBootstrapScriptForGroup(String group) {
> return groupToBootScript.apply(group);
Call `createBootstrapScriptForGroupAndNodeName(group, null)` once the `GroupToBootScript` function is changed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21201988
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by William Chu <no...@github.com>.
Ok, please review at your convenience and let me know if you see anything else
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65331605
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1978](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1978/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65366025
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by William Chu <no...@github.com>.
> @@ -171,8 +172,8 @@ String buildBootstrapConfiguration(BootstrapConfig bootstrapConfig) {
> }
>
> @Override
> - public Statement createBootstrapScriptForGroup(String group) {
> - return groupToBootScript.apply(group);
> + public Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName) {
> + return groupToBootScript.apply(group, null);
2 methods added to the implementing class, BaseChefService, as well as the interface class, ChefService.
> On Dec 4, 2014, at 9:46 AM, Ignasi Barrera <no...@github.com> wrote:
>
> In apis/chef/src/main/java/org/jclouds/chef/internal/BaseChefService.java:
>
> > @@ -171,8 +172,8 @@ String buildBootstrapConfiguration(BootstrapConfig bootstrapConfig) {
> > }
> >
> > @Override
> > - public Statement createBootstrapScriptForGroup(String group) {
> > - return groupToBootScript.apply(group);
> > + public Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName) {
> > + return groupToBootScript.apply(group, null);
> No. I think I didn't explain properly. There must be two methods. The old and the new one so we don't break existing code.
>
> The previous comment pretended to say this, remarking the fact that ot was that way in some previous revision of this PR.
>
> —
> Reply to this email directly or view it on GitHub <https://github.com/jclouds/jclouds/pull/622/files#r21321723>.
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21322472
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1981](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1981/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65513651
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by William Chu <no...@github.com>.
> @@ -171,8 +172,8 @@ String buildBootstrapConfiguration(BootstrapConfig bootstrapConfig) {
> }
>
> @Override
> - public Statement createBootstrapScriptForGroup(String group) {
> - return groupToBootScript.apply(group);
> + public Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName) {
> + return groupToBootScript.apply(group, null);
Ok, cool and committed. So just to be clear we’re fine with changing the method signature vs adding a second method that would support older implementations that are passing only one input parameter i.e. group?
> On Dec 4, 2014, at 7:57 AM, Ignasi Barrera <no...@github.com> wrote:
>
> In apis/chef/src/main/java/org/jclouds/chef/internal/BaseChefService.java:
>
> > @@ -171,8 +172,8 @@ String buildBootstrapConfiguration(BootstrapConfig bootstrapConfig) {
> > }
> >
> > @Override
> > - public Statement createBootstrapScriptForGroup(String group) {
> > - return groupToBootScript.apply(group);
> > + public Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName) {
> > + return groupToBootScript.apply(group, null);
> Absolutely. I thought there were the two methods in some previous commit...
>
> —
> Reply to this email directly or view it on GitHub <https://github.com/jclouds/jclouds/pull/622/files#r21313028>.
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21320153
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -171,8 +172,8 @@ String buildBootstrapConfiguration(BootstrapConfig bootstrapConfig) {
> }
>
> @Override
> - public Statement createBootstrapScriptForGroup(String group) {
> - return groupToBootScript.apply(group);
> + public Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName) {
> + return groupToBootScript.apply(group, null);
Absolutely. I thought there were the two methods in some previous commit...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21313028
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by William Chu <no...@github.com>.
> @@ -171,8 +172,8 @@ String buildBootstrapConfiguration(BootstrapConfig bootstrapConfig) {
> }
>
> @Override
> - public Statement createBootstrapScriptForGroup(String group) {
> - return groupToBootScript.apply(group);
> + public Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName) {
> + return groupToBootScript.apply(group, null);
Shouldn't the second parameter be nodeName and not null? I was thinking about this more. Shouldn't the ChefService interface have 2 methods? One that accepts one parameter, group and another that accepts two parameters, group and node name? Otherwise the signature change would force others to change their code?
Thanks,
Will
> On Dec 4, 2014, at 02:36, Ignasi Barrera <no...@github.com> wrote:
>
> In apis/chef/src/main/java/org/jclouds/chef/internal/BaseChefService.java:
>
> > @@ -171,8 +172,8 @@ String buildBootstrapConfiguration(BootstrapConfig bootstrapConfig) {
> > }
> >
> > @Override
> > - public Statement createBootstrapScriptForGroup(String group) {
> > - return groupToBootScript.apply(group);
> > + public Statement createBootstrapScriptForGroup(String group, @Nullable String nodeName) {
> > + return groupToBootScript.apply(group, null);
> Hmmm this is wrong now? There should be two methods, one passing null and the other one passing the nodeName.
>
> —
> Reply to this email directly or view it on GitHub.
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21312577
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1985](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1985/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65677146
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1977](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1977/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65346601
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -103,9 +102,12 @@ public Statement apply(String group) {
>
> String chefConfigDir = "{root}etc{fs}chef";
> Statement createChefConfigDir = exec("{md} " + chefConfigDir);
> + String createNodeName = String.format("node_name \"%s-\" + o[:ipaddress]", group);
> + if (nodeName != null) {
> + createNodeName = String.format("node_name \"%s\"", nodeName);
> + }
Can you change it to avoid creating a String object that you will potentially reassign immediately after? Just assign the right value directly with an if/else clause.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622/files#r21267422
Re: [jclouds] JCLOUDS-788 Added new method to support the passing of
a custom node nam... (#622)
Posted by Ignasi Barrera <no...@github.com>.
Squashed the commits and committed to [master](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=06653f1dd2af12e66301b42734d6b9d8ada7a472) and [1.8.x](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=125d93aa582530f5e790dd20c1c28c4fc07083f4).
Thanks @will-districtnerds!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/622#issuecomment-65723297