You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by grkvlt <gi...@git.apache.org> on 2014/07/24 17:53:02 UTC

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

GitHub user grkvlt opened a pull request:

    https://github.com/apache/incubator-brooklyn/pull/94

    BROOKLYN-12 Node.JS Entity

    New entity implementing a Node.JS application. 

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/grkvlt/incubator-brooklyn feature/nodejs

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-brooklyn/pull/94.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #94
    
----
commit 3ad3470195694bb0b83f61bc314925a2c598c13b
Author: Andrew Kennedy <an...@cloudsoftcorp.com>
Date:   2014-06-03T21:43:42Z

    Initial NodeJS entity

commit 257aaaf3676d9424d7d50da1bb75514d77956521
Author: Andrew Kennedy <gr...@apache.org>
Date:   2014-07-24T15:49:50Z

    Updated NodeJS entity

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15810286
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/webapp/JavaWebAppSoftwareProcessImpl.java ---
    @@ -178,33 +176,4 @@ protected void doStop() {
             setAttribute(REQUESTS_PER_SECOND_LAST, 0D);
             setAttribute(REQUESTS_PER_SECOND_IN_WINDOW, 0D);
         }
    -    
    -    protected Set<String> getEnabledProtocols() {
    -        return getAttribute(JavaWebAppSoftwareProcess.ENABLED_PROTOCOLS);
    -    }
    -    
    -    protected boolean isProtocolEnabled(String protocol) {
    -        for (String contender : getEnabledProtocols()) {
    -            if (protocol.equalsIgnoreCase(contender)) {
    -                return true;
    -            }
    -        }
    -        return false;
    -    }
    -    
    -    protected String inferBrooklynAccessibleRootUrl() {
    --- End diff --
    
    Did nothing use this method?! 
    
    Should we change things like:
    
        HostAndPort accessible = BrooklynAccessUtils.getBrooklynAccessibleAddress(this, getAttribute(HTTP_PORT));
        String nodeJsUrl = String.format("http://%s:%d", accessible.getHostText(), accessible.getPort());
    
    to use it instead, or should we deprecate it and go with the pattern used elsewhere?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15812045
  
    --- Diff: software/webapp/src/test/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppSimpleIntegrationTest.java ---
    @@ -0,0 +1,100 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import static org.testng.Assert.assertFalse;
    +import static org.testng.Assert.fail;
    +
    +import java.net.ServerSocket;
    +import java.util.Iterator;
    +
    +import org.jclouds.util.Throwables2;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +import org.testng.annotations.AfterMethod;
    +import org.testng.annotations.BeforeMethod;
    +import org.testng.annotations.Test;
    +
    +import brooklyn.entity.basic.ApplicationBuilder;
    +import brooklyn.entity.basic.Entities;
    +import brooklyn.entity.proxying.EntitySpec;
    +import brooklyn.location.PortRange;
    +import brooklyn.location.basic.LocalhostMachineProvisioningLocation;
    +import brooklyn.location.basic.PortRanges;
    +import brooklyn.test.entity.TestApplication;
    +import brooklyn.util.net.Networking;
    +
    +import com.google.common.collect.ImmutableList;
    +
    +/**
    + * This tests the operation of the {@link NodeJsWebAppService} entity.
    + *
    + * FIXME this test is largely superseded by WebApp*IntegrationTest which tests inter alia Tomcat
    + */
    +public class NodeJsWebAppSimpleIntegrationTest {
    +    @SuppressWarnings("unused")
    +    private static final Logger LOG = LoggerFactory.getLogger(NodeJsWebAppSimpleIntegrationTest.class);
    +
    +    /** don't use 8080 since that is commonly used by testing software; use different from other tests. */
    +    static PortRange DEFAULT_HTTP_PORT_RANGE = PortRanges.fromString("7880-7980");
    +
    +    private TestApplication app;
    +    private NodeJsWebAppService nodejs;
    +    private int httpPort;
    +
    +    @BeforeMethod(alwaysRun=true)
    +    public void pickFreePort() {
    +        for (Iterator<Integer> iter = DEFAULT_HTTP_PORT_RANGE.iterator(); iter.hasNext();) {
    +            Integer port = iter.next();
    +            if (Networking.isPortAvailable(port)) {
    +                httpPort = port;
    +                return;
    +            }
    +        }
    +        fail("someone is already listening on ports "+DEFAULT_HTTP_PORT_RANGE+"; tests assume that port is free on localhost");
    +    }
    +
    +    @AfterMethod(alwaysRun=true)
    +    public void tearDown() throws Exception {
    +        if (app != null) Entities.destroyAll(app.getManagementContext());
    +    }
    +
    +    @Test(groups="Integration")
    +    public void detectFailureIfTomcanodejsantBindToPort() throws Exception {
    --- End diff --
    
    Remove "Tomca" from name.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r16298434
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppSshDriver.java ---
    @@ -0,0 +1,161 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import java.util.List;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.entity.basic.AbstractSoftwareProcessSshDriver;
    +import brooklyn.entity.basic.Attributes;
    +import brooklyn.entity.basic.SoftwareProcess;
    +import brooklyn.entity.webapp.WebAppService;
    +import brooklyn.location.basic.SshMachineLocation;
    +import brooklyn.util.collections.MutableList;
    +import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.file.ArchiveUtils;
    +import brooklyn.util.os.Os;
    +import brooklyn.util.ssh.BashCommands;
    +import brooklyn.util.text.Strings;
    +
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.ImmutableSet;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +
    +public class NodeJsWebAppSshDriver extends AbstractSoftwareProcessSshDriver implements NodeJsWebAppDriver {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(NodeJsWebAppService.class);
    +
    +    public NodeJsWebAppSshDriver(NodeJsWebAppServiceImpl entity, SshMachineLocation machine) {
    +        super(entity, machine);
    +    }
    +
    +    public NodeJsWebAppServiceImpl getEntity() {
    +        return (NodeJsWebAppServiceImpl) super.getEntity();
    +    }
    +
    +    @Override
    +    public Integer getHttpPort() {
    +        return getEntity().getAttribute(Attributes.HTTP_PORT);
    +    }
    +
    +    @Override
    +    public void postLaunch() {
    +        String rootUrl = String.format("http://%s:%d/", getHostname(), getHttpPort());
    +        entity.setAttribute(WebAppService.ROOT_URL, rootUrl);
    +    }
    +
    +    protected Map<String, Integer> getPortMap() {
    +        return ImmutableMap.of("http", getEntity().getAttribute(WebAppService.HTTP_PORT));
    +    }
    +
    +    @Override
    +    public Set<Integer> getPortsUsed() {
    +        return ImmutableSet.<Integer>builder()
    +                .addAll(super.getPortsUsed())
    +                .addAll(getPortMap().values())
    +                .build();
    +    }
    +
    +    @Override
    +    public void install() {
    +        List<String> packages = getEntity().getConfig(NodeJsWebAppService.NODE_PACKAGE_LIST);
    +        LOG.info("Installing Node.JS {} {}", getEntity().getConfig(SoftwareProcess.SUGGESTED_VERSION), Iterables.toString(packages));
    +
    +        List<String> commands = MutableList.<String>builder()
    --- End diff --
    
    In this case we're adding extra commands conditionally, so the list must be mutable, otherwise agree.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15810535
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppSshDriver.java ---
    @@ -0,0 +1,161 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import java.util.List;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.entity.basic.AbstractSoftwareProcessSshDriver;
    +import brooklyn.entity.basic.Attributes;
    +import brooklyn.entity.basic.SoftwareProcess;
    +import brooklyn.entity.webapp.WebAppService;
    +import brooklyn.location.basic.SshMachineLocation;
    +import brooklyn.util.collections.MutableList;
    +import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.file.ArchiveUtils;
    +import brooklyn.util.os.Os;
    +import brooklyn.util.ssh.BashCommands;
    +import brooklyn.util.text.Strings;
    +
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.ImmutableSet;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +
    +public class NodeJsWebAppSshDriver extends AbstractSoftwareProcessSshDriver implements NodeJsWebAppDriver {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(NodeJsWebAppService.class);
    +
    +    public NodeJsWebAppSshDriver(NodeJsWebAppServiceImpl entity, SshMachineLocation machine) {
    +        super(entity, machine);
    +    }
    +
    +    public NodeJsWebAppServiceImpl getEntity() {
    +        return (NodeJsWebAppServiceImpl) super.getEntity();
    +    }
    +
    +    @Override
    +    public Integer getHttpPort() {
    +        return getEntity().getAttribute(Attributes.HTTP_PORT);
    +    }
    +
    +    @Override
    +    public void postLaunch() {
    +        String rootUrl = String.format("http://%s:%d/", getHostname(), getHttpPort());
    +        entity.setAttribute(WebAppService.ROOT_URL, rootUrl);
    +    }
    +
    +    protected Map<String, Integer> getPortMap() {
    +        return ImmutableMap.of("http", getEntity().getAttribute(WebAppService.HTTP_PORT));
    +    }
    +
    +    @Override
    +    public Set<Integer> getPortsUsed() {
    +        return ImmutableSet.<Integer>builder()
    +                .addAll(super.getPortsUsed())
    +                .addAll(getPortMap().values())
    +                .build();
    +    }
    +
    +    @Override
    +    public void install() {
    +        List<String> packages = getEntity().getConfig(NodeJsWebAppService.NODE_PACKAGE_LIST);
    +        LOG.info("Installing Node.JS {} {}", getEntity().getConfig(SoftwareProcess.SUGGESTED_VERSION), Iterables.toString(packages));
    +
    +        List<String> commands = MutableList.<String>builder()
    +                .add(BashCommands.INSTALL_CURL)
    +                .add(BashCommands.ifExecutableElse0("apt-get", BashCommands.chain(
    +                        BashCommands.installPackage("python-software-properties python g++ make"),
    +                        BashCommands.sudo("add-apt-repository ppa:chris-lea/node.js"))))
    +                .add(BashCommands.installPackage(MutableMap.of("yum", "git nodejs npm", "apt", "git-core nodejs"), null))
    +                .add(BashCommands.sudo("npm install -g n"))
    +                .add(BashCommands.sudo("n " + getEntity().getConfig(SoftwareProcess.SUGGESTED_VERSION)))
    +                .build();
    +
    +        if (packages != null && packages.size() > 0) {
    +            commands.add(BashCommands.sudo("npm install -g " + Joiner.on(' ').join(packages)));
    +        }
    +
    +        newScript(INSTALLING)
    +                .body.append(commands)
    +                .execute();
    +    }
    +
    +    @Override
    +    public void customize() {
    +        List<String> commands = Lists.newLinkedList();
    +
    +        String gitRepoUrl = getEntity().getConfig(NodeJsWebAppService.APP_GIT_REPOSITORY_URL);
    +        String archiveUrl = getEntity().getConfig(NodeJsWebAppService.APP_ARCHIVE_URL);
    +        String appName = getEntity().getConfig(NodeJsWebAppService.APP_NAME);
    +
    +        if (Strings.isNonBlank(gitRepoUrl) && Strings.isNonBlank(archiveUrl)) {
    +            throw new IllegalStateException("Only one of Git or archive URL must be set");
    --- End diff --
    
    Include which entity this is in the exception (and also what the gitRepoUrl and archiveUrl are). e.g.
    
        throw new IllegalStateException("Only one of Git or archive URL must be set for "+entity+"; got repo "+gitRepoUrl+" and archive "+archiveUrl);
    
    Same for other exception thrown below.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-brooklyn/pull/94


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15810605
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppSshDriver.java ---
    @@ -0,0 +1,161 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import java.util.List;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.entity.basic.AbstractSoftwareProcessSshDriver;
    +import brooklyn.entity.basic.Attributes;
    +import brooklyn.entity.basic.SoftwareProcess;
    +import brooklyn.entity.webapp.WebAppService;
    +import brooklyn.location.basic.SshMachineLocation;
    +import brooklyn.util.collections.MutableList;
    +import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.file.ArchiveUtils;
    +import brooklyn.util.os.Os;
    +import brooklyn.util.ssh.BashCommands;
    +import brooklyn.util.text.Strings;
    +
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.ImmutableSet;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +
    +public class NodeJsWebAppSshDriver extends AbstractSoftwareProcessSshDriver implements NodeJsWebAppDriver {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(NodeJsWebAppService.class);
    +
    +    public NodeJsWebAppSshDriver(NodeJsWebAppServiceImpl entity, SshMachineLocation machine) {
    +        super(entity, machine);
    +    }
    +
    +    public NodeJsWebAppServiceImpl getEntity() {
    +        return (NodeJsWebAppServiceImpl) super.getEntity();
    +    }
    +
    +    @Override
    +    public Integer getHttpPort() {
    +        return getEntity().getAttribute(Attributes.HTTP_PORT);
    +    }
    +
    +    @Override
    +    public void postLaunch() {
    +        String rootUrl = String.format("http://%s:%d/", getHostname(), getHttpPort());
    +        entity.setAttribute(WebAppService.ROOT_URL, rootUrl);
    +    }
    +
    +    protected Map<String, Integer> getPortMap() {
    +        return ImmutableMap.of("http", getEntity().getAttribute(WebAppService.HTTP_PORT));
    +    }
    +
    +    @Override
    +    public Set<Integer> getPortsUsed() {
    +        return ImmutableSet.<Integer>builder()
    +                .addAll(super.getPortsUsed())
    +                .addAll(getPortMap().values())
    +                .build();
    +    }
    +
    +    @Override
    +    public void install() {
    +        List<String> packages = getEntity().getConfig(NodeJsWebAppService.NODE_PACKAGE_LIST);
    +        LOG.info("Installing Node.JS {} {}", getEntity().getConfig(SoftwareProcess.SUGGESTED_VERSION), Iterables.toString(packages));
    +
    +        List<String> commands = MutableList.<String>builder()
    +                .add(BashCommands.INSTALL_CURL)
    +                .add(BashCommands.ifExecutableElse0("apt-get", BashCommands.chain(
    +                        BashCommands.installPackage("python-software-properties python g++ make"),
    +                        BashCommands.sudo("add-apt-repository ppa:chris-lea/node.js"))))
    +                .add(BashCommands.installPackage(MutableMap.of("yum", "git nodejs npm", "apt", "git-core nodejs"), null))
    +                .add(BashCommands.sudo("npm install -g n"))
    +                .add(BashCommands.sudo("n " + getEntity().getConfig(SoftwareProcess.SUGGESTED_VERSION)))
    +                .build();
    +
    +        if (packages != null && packages.size() > 0) {
    +            commands.add(BashCommands.sudo("npm install -g " + Joiner.on(' ').join(packages)));
    +        }
    +
    +        newScript(INSTALLING)
    +                .body.append(commands)
    +                .execute();
    +    }
    +
    +    @Override
    +    public void customize() {
    +        List<String> commands = Lists.newLinkedList();
    +
    +        String gitRepoUrl = getEntity().getConfig(NodeJsWebAppService.APP_GIT_REPOSITORY_URL);
    +        String archiveUrl = getEntity().getConfig(NodeJsWebAppService.APP_ARCHIVE_URL);
    +        String appName = getEntity().getConfig(NodeJsWebAppService.APP_NAME);
    +
    +        if (Strings.isNonBlank(gitRepoUrl) && Strings.isNonBlank(archiveUrl)) {
    +            throw new IllegalStateException("Only one of Git or archive URL must be set");
    +        } else if (Strings.isNonBlank(gitRepoUrl)) {
    +            commands.add(String.format("git clone %s %s", gitRepoUrl, appName));
    +        } else if (Strings.isNonBlank(archiveUrl)) {
    +            ArchiveUtils.deploy(archiveUrl, getMachine(), getRunDir());
    +        } else {
    +            throw new IllegalStateException("At least one of Git or archive URL must be set");
    +        }
    +
    +        newScript(CUSTOMIZING)
    +                .body.append(commands)
    +                .execute();
    +    }
    +
    +    @Override
    +    public void launch() {
    +        List<String> commands = Lists.newLinkedList();
    +
    +        String appName = getEntity().getConfig(NodeJsWebAppService.APP_NAME);
    --- End diff --
    
    I've added (in too many places!):
    
        protected <K> K getRequiredConfig(ConfigKey<K> key) {
            return checkNotNull(getConfig(key), key.getName());
        }
    
    I presume this will fail in a strange way if any of those are null (i.e. command to exec will contain the string `"null"`).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15810863
  
    --- Diff: software/webapp/src/test/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppFixtureIntegrationTest.java ---
    @@ -0,0 +1,62 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +import org.testng.annotations.DataProvider;
    +
    +import brooklyn.entity.basic.SoftwareProcess;
    +import brooklyn.entity.proxying.EntitySpec;
    +import brooklyn.entity.webapp.AbstractWebAppFixtureIntegrationTest;
    +import brooklyn.entity.webapp.WebAppService;
    +import brooklyn.location.basic.PortRanges;
    +import brooklyn.test.entity.TestApplication;
    +
    +public class NodeJsWebAppFixtureIntegrationTest extends AbstractWebAppFixtureIntegrationTest {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(NodeJsWebAppFixtureIntegrationTest.class);
    +
    +    public static final String GIT_REPO_URL = "https://github.com/grkvlt/node-hello-world.git";
    --- End diff --
    
    Do you think we should move this so it's not owned by an individual? No strong feelings as it's only used by a test. But if it ever gets into a tutorial then we should move it into something like `github:brooklyncentral/node-hello-world.git`



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15809988
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/webapp/JavaWebAppSoftwareProcessImpl.java ---
    @@ -178,33 +176,4 @@ protected void doStop() {
             setAttribute(REQUESTS_PER_SECOND_LAST, 0D);
             setAttribute(REQUESTS_PER_SECOND_IN_WINDOW, 0D);
         }
    -    
    -    protected Set<String> getEnabledProtocols() {
    -        return getAttribute(JavaWebAppSoftwareProcess.ENABLED_PROTOCOLS);
    -    }
    -    
    -    protected boolean isProtocolEnabled(String protocol) {
    -        for (String contender : getEnabledProtocols()) {
    -            if (protocol.equalsIgnoreCase(contender)) {
    -                return true;
    -            }
    -        }
    -        return false;
    -    }
    -    
    -    protected String inferBrooklynAccessibleRootUrl() {
    --- End diff --
    
    Agree to moving this code to somewhere that's not java specific (i.e. not in `JavaWebAppSoftwareProcessImpl`). However, we should probably deprecate this method and delete after releasing 0.7.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#issuecomment-51203300
  
    Code looks good; just minor comments.
    
    However, the live test `NodeJsWebAppSoftlayerLiveTest.test_Ubuntu_12_0_4` failed for me. I got the error below. No idea why `ps -p` would be complaining, as it looks like it should have populated the pid file. The test terminated the VM so I haven't investigated further.
    
        2014-08-05 14:57:58,570 DEBUG brooklyn.SSH [brooklyn-execmanager-m55tvPWd-10]: launching NodeJsWebAppServiceImpl{id=J0xgqOS2}, initiating ssh on machine SshMachineLocation[119.81.168.42:119.81.168.42/119.81.168.42] (env {PORT=8080}): export RUN_DIR="/home/users/aled/brooklyn-managed-processes/apps/mU7cP0SC/entities/NodeJsWebAppService_J0xgqOS2" ; mkdir -p $RUN_DIR ; cd $RUN_DIR ; cd /home/users/aled/brooklyn-managed-processes/apps/mU7cP0SC/entities/NodeJsWebAppService_J0xgqOS2/node-hello-world ; ( if test "$UID" -eq 0; then ( nohup node app.js & ); else echo "nohup node app.js &" | sudo -E -n -S -s -- bash ; fi ) ; echo $! > /home/users/aled/brooklyn-managed-processes/apps/mU7cP0SC/entities/NodeJsWebAppService_J0xgqOS2/pid.txt
        2014-08-05 14:58:04,574 DEBUG brooklyn.SSH [Thread-32]: [J0xgqOS2@119.81.168.42:stdout] Executed /tmp/brooklyn-20140805-145758571-jaxX-launching_NodeJsWebAppServiceI.sh, result 0
    
    
        2014-08-05 14:58:05,527 DEBUG brooklyn.SSH [brooklyn-execmanager-m55tvPWd-8]: check-running NodeJsWebAppServiceImpl{id=J0xgqOS2}, initiating ssh on machine SshMachineLocation[119.81.168.42:119.81.168.42/119.81.168.42] (env {PORT=8080}): export RUN_DIR="/home/users/aled/brooklyn-managed-processes/apps/mU7cP0SC/entities/NodeJsWebAppService_J0xgqOS2" ; mkdir -p $RUN_DIR ; cd $RUN_DIR ; test -f /home/users/aled/brooklyn-managed-processes/apps/mU7cP0SC/entities/NodeJsWebAppService_J0xgqOS2/pid.txt || exit 1 ; ps -p `cat /home/users/aled/brooklyn-managed-processes/apps/mU7cP0SC/entities/NodeJsWebAppService_J0xgqOS2/pid.txt`
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] ERROR: List of process IDs must follow -p.
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] ********* simple selection *********  ********* selection by list *********
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -A all processes                      -C by command name
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -N negate selection                   -G by real group ID (supports names)
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -a all w/ tty except session leaders  -U by real user ID (supports names)
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -d all except session leaders         -g by session OR by effective group name
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -e all processes                      -p by process ID
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] T  all processes on this terminal     -s processes in the sessions given
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] a  all w/ tty, including other users  -t by tty
        2014-08-05 14:58:12,723 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] g  OBSOLETE -- DO NOT USE             -u by effective user ID (supports names)
        2014-08-05 14:58:12,724 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] r  only running processes             U  processes for specified users
        2014-08-05 14:58:12,724 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] x  processes w/o controlling ttys     t  by tty
        2014-08-05 14:58:12,724 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] *********** output format **********  *********** long options ***********
        2014-08-05 14:58:12,724 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -o,o user-defined  -f full            --Group --User --pid --cols --ppid
        2014-08-05 14:58:12,724 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -j,j job control   s  signal          --group --user --sid --rows --info
        2014-08-05 14:58:13,069 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -O,O preloaded -o  v  virtual memory  --cumulative --format --deselect
        2014-08-05 14:58:13,069 DEBUG brooklyn.SSH [Thread-37]: [J0xgqOS2@119.81.168.42:stdout] Executed /tmp/brooklyn-20140805-145808397-X4DC-check-running_NodeJsWebAppServ.sh, result 1
        2014-08-05 14:58:13,069 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -l,l long          u  user-oriented   --sort --tty --forest --version
        2014-08-05 14:58:13,070 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -F   extra full    X  registers       --heading --no-heading --context
        2014-08-05 14:58:13,070 DEBUG brooklyn.SSH [brooklyn-execmanager-m55tvPWd-8]: check-running NodeJsWebAppServiceImpl{id=J0xgqOS2}, on machine SshMachineLocation[119.81.168.42:119.81.168.42/119.81.168.42], completed: return status 1
        2014-08-05 14:58:13,071 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr]                     ********* misc options *********
        2014-08-05 14:58:13,071 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -V,V  show version      L  list format codes  f  ASCII art forest
        2014-08-05 14:58:13,071 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -m,m,-L,-T,H  threads   S  children in sum    -y change -l format
        2014-08-05 14:58:13,071 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -M,Z  security data     c  true command name  -c scheduling class
        2014-08-05 14:58:13,071 DEBUG brooklyn.SSH [Thread-38]: [J0xgqOS2@119.81.168.42:stderr] -w,w  wide output       n  numeric WCHAN,UID  -H process hierarchy



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#issuecomment-51193138
  
    Hi @grkvlt can you squash the commits for future PRs please (e.g. someone looking back at the history won't find "change root URL to use default" or "Add APACHE-2.0 license header" as particularly helpful. Particularly the latter should be squashed with the commit that added the files originally.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15812712
  
    --- Diff: software/webapp/src/test/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppSimpleIntegrationTest.java ---
    @@ -0,0 +1,100 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import static org.testng.Assert.assertFalse;
    +import static org.testng.Assert.fail;
    +
    +import java.net.ServerSocket;
    +import java.util.Iterator;
    +
    +import org.jclouds.util.Throwables2;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +import org.testng.annotations.AfterMethod;
    +import org.testng.annotations.BeforeMethod;
    +import org.testng.annotations.Test;
    +
    +import brooklyn.entity.basic.ApplicationBuilder;
    +import brooklyn.entity.basic.Entities;
    +import brooklyn.entity.proxying.EntitySpec;
    +import brooklyn.location.PortRange;
    +import brooklyn.location.basic.LocalhostMachineProvisioningLocation;
    +import brooklyn.location.basic.PortRanges;
    +import brooklyn.test.entity.TestApplication;
    +import brooklyn.util.net.Networking;
    +
    +import com.google.common.collect.ImmutableList;
    +
    +/**
    + * This tests the operation of the {@link NodeJsWebAppService} entity.
    + *
    + * FIXME this test is largely superseded by WebApp*IntegrationTest which tests inter alia Tomcat
    + */
    +public class NodeJsWebAppSimpleIntegrationTest {
    --- End diff --
    
    Test fails on OS X - no brew or port equivalents for install. Need to document that somewhere (perhaps both here in the test, and on the javadoc of the entity itself).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15810458
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppSshDriver.java ---
    @@ -0,0 +1,161 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import java.util.List;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.entity.basic.AbstractSoftwareProcessSshDriver;
    +import brooklyn.entity.basic.Attributes;
    +import brooklyn.entity.basic.SoftwareProcess;
    +import brooklyn.entity.webapp.WebAppService;
    +import brooklyn.location.basic.SshMachineLocation;
    +import brooklyn.util.collections.MutableList;
    +import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.file.ArchiveUtils;
    +import brooklyn.util.os.Os;
    +import brooklyn.util.ssh.BashCommands;
    +import brooklyn.util.text.Strings;
    +
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.ImmutableSet;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +
    +public class NodeJsWebAppSshDriver extends AbstractSoftwareProcessSshDriver implements NodeJsWebAppDriver {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(NodeJsWebAppService.class);
    +
    +    public NodeJsWebAppSshDriver(NodeJsWebAppServiceImpl entity, SshMachineLocation machine) {
    +        super(entity, machine);
    +    }
    +
    +    public NodeJsWebAppServiceImpl getEntity() {
    +        return (NodeJsWebAppServiceImpl) super.getEntity();
    +    }
    +
    +    @Override
    +    public Integer getHttpPort() {
    +        return getEntity().getAttribute(Attributes.HTTP_PORT);
    +    }
    +
    +    @Override
    +    public void postLaunch() {
    +        String rootUrl = String.format("http://%s:%d/", getHostname(), getHttpPort());
    +        entity.setAttribute(WebAppService.ROOT_URL, rootUrl);
    +    }
    +
    +    protected Map<String, Integer> getPortMap() {
    +        return ImmutableMap.of("http", getEntity().getAttribute(WebAppService.HTTP_PORT));
    +    }
    +
    +    @Override
    +    public Set<Integer> getPortsUsed() {
    +        return ImmutableSet.<Integer>builder()
    +                .addAll(super.getPortsUsed())
    +                .addAll(getPortMap().values())
    +                .build();
    +    }
    +
    +    @Override
    +    public void install() {
    +        List<String> packages = getEntity().getConfig(NodeJsWebAppService.NODE_PACKAGE_LIST);
    +        LOG.info("Installing Node.JS {} {}", getEntity().getConfig(SoftwareProcess.SUGGESTED_VERSION), Iterables.toString(packages));
    +
    +        List<String> commands = MutableList.<String>builder()
    --- End diff --
    
    Personal preference for us to default to `ImmutableList` unless we really want to pass a null, or need the extra methods on the builder that aren't in the guava version.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15812615
  
    --- Diff: software/webapp/src/test/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppSimpleIntegrationTest.java ---
    @@ -0,0 +1,100 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import static org.testng.Assert.assertFalse;
    +import static org.testng.Assert.fail;
    +
    +import java.net.ServerSocket;
    +import java.util.Iterator;
    +
    +import org.jclouds.util.Throwables2;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +import org.testng.annotations.AfterMethod;
    +import org.testng.annotations.BeforeMethod;
    +import org.testng.annotations.Test;
    +
    +import brooklyn.entity.basic.ApplicationBuilder;
    +import brooklyn.entity.basic.Entities;
    +import brooklyn.entity.proxying.EntitySpec;
    +import brooklyn.location.PortRange;
    +import brooklyn.location.basic.LocalhostMachineProvisioningLocation;
    +import brooklyn.location.basic.PortRanges;
    +import brooklyn.test.entity.TestApplication;
    +import brooklyn.util.net.Networking;
    +
    +import com.google.common.collect.ImmutableList;
    +
    +/**
    + * This tests the operation of the {@link NodeJsWebAppService} entity.
    + *
    + * FIXME this test is largely superseded by WebApp*IntegrationTest which tests inter alia Tomcat
    --- End diff --
    
    Delete this fixme?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#issuecomment-52940407
  
    Ready to merge. Tested with a couple of example applications, a basic Hello World and a Todo manager using Redis.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15810326
  
    --- Diff: software/webapp/src/main/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppSshDriver.java ---
    @@ -0,0 +1,161 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import java.util.List;
    +import java.util.Map;
    +import java.util.Set;
    +
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.entity.basic.AbstractSoftwareProcessSshDriver;
    +import brooklyn.entity.basic.Attributes;
    +import brooklyn.entity.basic.SoftwareProcess;
    +import brooklyn.entity.webapp.WebAppService;
    +import brooklyn.location.basic.SshMachineLocation;
    +import brooklyn.util.collections.MutableList;
    +import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.file.ArchiveUtils;
    +import brooklyn.util.os.Os;
    +import brooklyn.util.ssh.BashCommands;
    +import brooklyn.util.text.Strings;
    +
    +import com.google.common.base.Joiner;
    +import com.google.common.collect.ImmutableMap;
    +import com.google.common.collect.ImmutableSet;
    +import com.google.common.collect.Iterables;
    +import com.google.common.collect.Lists;
    +
    +public class NodeJsWebAppSshDriver extends AbstractSoftwareProcessSshDriver implements NodeJsWebAppDriver {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(NodeJsWebAppService.class);
    +
    +    public NodeJsWebAppSshDriver(NodeJsWebAppServiceImpl entity, SshMachineLocation machine) {
    +        super(entity, machine);
    +    }
    +
    +    public NodeJsWebAppServiceImpl getEntity() {
    +        return (NodeJsWebAppServiceImpl) super.getEntity();
    +    }
    +
    +    @Override
    +    public Integer getHttpPort() {
    +        return getEntity().getAttribute(Attributes.HTTP_PORT);
    +    }
    +
    +    @Override
    +    public void postLaunch() {
    +        String rootUrl = String.format("http://%s:%d/", getHostname(), getHttpPort());
    +        entity.setAttribute(WebAppService.ROOT_URL, rootUrl);
    +    }
    +
    +    protected Map<String, Integer> getPortMap() {
    +        return ImmutableMap.of("http", getEntity().getAttribute(WebAppService.HTTP_PORT));
    --- End diff --
    
    Use `getHttpPort()` instead of repeating call to `getAttribute()`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: BROOKLYN-12 Node.JS Entity

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/94#discussion_r15812020
  
    --- Diff: software/webapp/src/test/java/brooklyn/entity/webapp/nodejs/NodeJsWebAppSimpleIntegrationTest.java ---
    @@ -0,0 +1,100 @@
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +package brooklyn.entity.webapp.nodejs;
    +
    +import static org.testng.Assert.assertFalse;
    +import static org.testng.Assert.fail;
    +
    +import java.net.ServerSocket;
    +import java.util.Iterator;
    +
    +import org.jclouds.util.Throwables2;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +import org.testng.annotations.AfterMethod;
    +import org.testng.annotations.BeforeMethod;
    +import org.testng.annotations.Test;
    +
    +import brooklyn.entity.basic.ApplicationBuilder;
    +import brooklyn.entity.basic.Entities;
    +import brooklyn.entity.proxying.EntitySpec;
    +import brooklyn.location.PortRange;
    +import brooklyn.location.basic.LocalhostMachineProvisioningLocation;
    +import brooklyn.location.basic.PortRanges;
    +import brooklyn.test.entity.TestApplication;
    +import brooklyn.util.net.Networking;
    +
    +import com.google.common.collect.ImmutableList;
    +
    +/**
    + * This tests the operation of the {@link NodeJsWebAppService} entity.
    + *
    + * FIXME this test is largely superseded by WebApp*IntegrationTest which tests inter alia Tomcat
    + */
    +public class NodeJsWebAppSimpleIntegrationTest {
    +    @SuppressWarnings("unused")
    +    private static final Logger LOG = LoggerFactory.getLogger(NodeJsWebAppSimpleIntegrationTest.class);
    +
    +    /** don't use 8080 since that is commonly used by testing software; use different from other tests. */
    +    static PortRange DEFAULT_HTTP_PORT_RANGE = PortRanges.fromString("7880-7980");
    +
    +    private TestApplication app;
    +    private NodeJsWebAppService nodejs;
    +    private int httpPort;
    +
    +    @BeforeMethod(alwaysRun=true)
    +    public void pickFreePort() {
    --- End diff --
    
    We should move the body of this into a common untility, something similar to `Networking.nextAvailablePort(int port)`. For example the code below:
    
        public static int firstAvailablePort(PortRange range) {
            for (Integer port : range) {
                if (Networking.isPortAvailable(port)) {
                    return port;
                }
            }
            fail("someone is already listening on ports "+range+" on localhost");
        }
    
    Unfortunately it can't go in `Networking` as `PortRange` isn't visible. We could consider moving PortRange and PortRanges into `brooklyn-utils-common`, next to `Networking`. But not in this PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---