Add configuration for setting the Jenkins URL. #13

tkuhlman · 2017-01-05T00:34:55Z

This change adds the public_url charm configuration option so you can
specify the Jenkins URL and prefix.

ryan-beisner · 2017-01-05T04:48:32Z

There should be corresponding unit and amulet test coverage for the new config option. This reactive rewrite has pretty good unit and amulet test coverage and I'd really like to see us keep that level of trust as contributions are made. Thank you.

tkuhlman · 2017-01-05T16:08:06Z

There are corresponding unit tests in my submission.

As far as amulet tests, I couldn't get them to work with Juju2 and I have no Juju1 environments anymore. In my dev instead I used an mojo spec to test the full integration, I was hoping Travis would exercise the amulet tests for me, but I see that isn't the case. Is there a place where I can run amulet test any changes or a trick to getting them to work with Juju2? The bug I hit is https://bugs.launchpad.net/juju/+bug/1608723

The basic amulet tests will of course use the default config value, I would just add changing it and verifying the change was made.

freeekanayaka · 2017-01-05T16:15:19Z

Tim Kuhlman <notifications@github.com> writes:

There are corresponding unit tests in my submission. As far as amulet tests, I couldn't get them to work with Juju2 and I have no Juju1 environments anymore.

They used to pass on Juju 2. Something has probably regressed, I'll have a look tomorrow.

In my dev instead I used an mojo spec to test the full integration, I was hoping Travis would exercise the amulet tests for me, but I see that isn't the case.

Yes, this a known pain point. There is some on going work from the juju-solutions team to provide easy glue between Jenkins and GitHub in order to run charm integration tests on custom Jenkins servers (Travis is unfortunately to limited to run juju integration tests properly).

Is there a place where I can run amulet test any changes or a trick to getting them to work with Juju2? The bug I hit is https://bugs.launchpad.net/juju/+bug/1608723

Good question. The situation has gone a bit wild since Juju 2 AFAIK, what I can say is that I use bundletester. Remember however that integration tests are supposed to be self-contained executables, so provided you have the required Python packages in your PATH and that you have a clean Juju 2 model at hand, you can just invoke: ./tests/NN_some_test

The basic amulet tests will of course use the default config value, I would just add changing it and verifying the change was made.

Sounds good.

tkuhlman · 2017-01-05T18:25:01Z

Thanks! I was able to get the amulet tests working with Juju2 by invoking the amulet tests directly which obviously I missed before. I'll add in some amulet tests later today.

freeekanayaka

Nice, I have a number of comments but mainly minor or stylistic.

I'll give the branch another pass once those are addressed.

freeekanayaka · 2017-01-06T14:21:41Z

.gitignore

@@ -6,3 +6,4 @@
 *.pyc
 lib/charms/__init__.py
 lib/charms/layer/__init__.py
+.idea


This looks like a leftover?

Out of habit I added that in since I use pycharm, I'll remove it.

freeekanayaka · 2017-01-06T14:23:11Z

config.yaml

@@ -48,3 +48,9 @@ options:
    default: 1
    description: |
      Number of executors to configure for jenkins master.
+  public_url:


Please use dashes as separator, like the other config keys in this file:

"public_url" -> "public-url"

freeekanayaka · 2017-01-06T14:24:16Z

config.yaml

+  public_url:
+    type: string
+    default: ""
+    description: >


Please use a pipe here (for consistencies with other config keys in this file):

"description: >" -> "description: |"

freeekanayaka · 2017-01-06T14:30:08Z

lib/charms/layer/jenkins/api.py

@@ -1,4 +1,5 @@
 import requests
+from urllib.parse import urlparse


Please follow PEP8 conventions, placing absolute imports before relative imports, and standard library imports before third-party and local library imports (all groups separated by a blank line):

import requests import jenkins from urllib.parse import urlparse from charmhelpers.core.hookenv import ERROR from charms.layer.jenkins.credentials import Credentials

freeekanayaka · 2017-01-06T14:35:34Z

lib/charms/layer/jenkins/api.py

+        prefix = urlparse(config["public_url"]).path
+        if not prefix:
+            prefix = "/"
+        return "http://localhost:8080{}".format(prefix)


Please take advantage of the URL constants defined in charms.layer.jenkins.service and use urljoin:

return urljoin(URL, perfix)

There is no URL constant in service anymore, I removed it. A constant is no longer valid with the ability to change prefix and it was only used by API calls so it makes more sense to be in the api file rather than in the service file. This method is the replacement for the constant.

freeekanayaka · 2017-01-06T14:45:09Z

lib/charms/layer/jenkins/configuration.py

@@ -11,8 +12,47 @@
 class Configuration(object):
    """Manage global Jenkins configuration."""

+    @staticmethod


There's no need for the @staticmethod decorator, please just add "self" to the signature and make it a regular instance method.

freeekanayaka · 2017-01-06T14:52:01Z

tests/basic.py

+    def test_10_change_url(self):
+        """Validate that after changing public_url we can login at the new
+            prefix and that the public_url has been updated.
+        """


Please format the docstring like this:

""" Validate that after changing public_url we can login at the new prefix and that the public_url has been updated. """

freeekanayaka · 2017-01-06T14:59:28Z

unit_tests/test_configuration.py

@@ -34,6 +36,46 @@ def test_bootstrap(self):
            FileContains(matcher=Contains("<numExecutors>1</numExecutors>")))
        self.assertEqual({8080}, self.fakes.juju.ports["TCP"])

+    def test_set_prefix(self):
+        test_file = tempfile.NamedTemporaryFile()


This file will be left around after the test suite completes. Please use something like:

from fixtures import TempDir # ... test_file = self.useFixture(TempDir()).join("test-file")

See the fixtures package for more details.

No NamedTemporaryFile defaults to deleting the file when it is closed so it doesn't hang around after the test suite completes. I'm happy to use the fixtures though.

Yeah, I should have mentioned that the case were it hangs around is if the test method fails. In that case the close() call never gets executed (an exception is raised before).

freeekanayaka · 2017-01-06T15:01:45Z

unit_tests/test_configuration.py

+        updated = self.configuration._set_prefix("")
+        self.assertTrue(updated)
+
+        # Case #6 - no config file, no expected change


Please break down all these separate cases in separate test methods. You'll probably want a small helper function to create the test file.

In this way if something breaks you can have an instant look at all edge cases that don't pass, instead of having to run the suite N times and discover a new failure each time.

freeekanayaka · 2017-01-06T15:03:03Z

unit_tests/test_users.py

@@ -33,6 +33,10 @@ def test_configure_admin_custom_password(self):
        If a password is provided, it's used to configure the admin user.
        """
        self.fakes.juju.config["password"] = "x"
+        # TODO figure out why self.fakes.juju setting is not reliably working.


Can you clarify this comment? I'll try to given an explanation or maybe fix an bug in the underlying machinery.

The problem here is I expected from looking at other code that self.fakes.juju.config["password"] would set the config option for the password but in my testing it did nothing it wasn't until I used hookenv.config() to get the config and set it that way that the test begun to work.

tkuhlman · 2017-01-06T23:52:55Z

I made updates as above, squashed all the commits together and rebased. I didn't end up switching NamedTemporaryFile for a fixture because there is only a temporary dir for the fixture so I still have to open a file an write to it anyway.

freeekanayaka

Looks good to me. I have a final comment (that I should have made from the beginning), feel free to address it or not.

freeekanayaka · 2017-01-09T17:43:00Z

unit_tests/test_configuration.py

+        """ Setup a temporary DEFAULTS_CONFIG_FILE
+        :return: a temporary file with the name attribute being the path
+        """
+        test_file_path = self.useFixture(fixtures.TempDir()).join("test-file")


Thanks for using the TempDir fixture, so no temporary files are left around when the test fails.

However, I just realized that I could have suggested you a way simpler approach with systemfixtures:

http://systemfixtures.readthedocs.io/en/latest/?badge=latest#filesystem

that's the pattern that the rest of the unit test code in this charm uses. In your case I think you would have to write something like:

self.fakes.fs.add(paths.DEFAULTS_CONFIG_FILE) os.makedirs("/etc/default")

and from this point any call to open("/etc/default/jenkins") will be magically redirected to a temporary file (something like "/tmp/tmpxxx/etc/default/jenkins).

You could add the code above to unit_tests.states.AptInstallJenkins, and since ConfigurationTest already uses the AptInstalledJenkins state fixtures all you unit tests will have that default file already setup (since it's a file that is always expected to be there in a real unit).

I realize the above is a tad tricky, if you have troubles getting it done let me know, or just land this PR as it is and I'll take care of this small clean up myself.

tkuhlman · 2017-01-09T18:23:48Z

Great that does clean up the tests nicely. I rebased and squashed to a single commit again since a new change just merged.

tkuhlman · 2017-01-09T18:31:09Z

Ah timing, I missed the fix from PR #16, which I am glad to see merge, one more rebase coming up.

freeekanayaka · 2017-01-09T19:04:50Z

Thanks, the change looks good to me. Do you have commit rights or should I trigger the merge myself?

tkuhlman · 2017-01-09T19:08:06Z

I don't have commit rights.

freeekanayaka requested changes Jan 6, 2017

View reviewed changes

freeekanayaka approved these changes Jan 9, 2017

View reviewed changes

freeekanayaka self-assigned this Jan 9, 2017

Add a configuration option that sets the Jenkins public url and prefix.

12dacb2

freeekanayaka merged commit 9ac1458 into jenkinsci:master Jan 9, 2017

tkuhlman deleted the public-url branch January 9, 2017 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration for setting the Jenkins URL. #13

Add configuration for setting the Jenkins URL. #13

tkuhlman commented Jan 5, 2017

ryan-beisner commented Jan 5, 2017

tkuhlman commented Jan 5, 2017

freeekanayaka commented Jan 5, 2017 via email

tkuhlman commented Jan 5, 2017

freeekanayaka left a comment

freeekanayaka Jan 6, 2017

tkuhlman Jan 6, 2017

freeekanayaka Jan 6, 2017

freeekanayaka Jan 6, 2017

freeekanayaka Jan 6, 2017

freeekanayaka Jan 6, 2017

tkuhlman Jan 6, 2017

freeekanayaka Jan 6, 2017

freeekanayaka Jan 6, 2017

freeekanayaka Jan 6, 2017

tkuhlman Jan 6, 2017

freeekanayaka Jan 6, 2017

freeekanayaka Jan 6, 2017

freeekanayaka Jan 6, 2017

tkuhlman Jan 6, 2017

tkuhlman commented Jan 6, 2017

freeekanayaka left a comment

freeekanayaka Jan 9, 2017

tkuhlman commented Jan 9, 2017

tkuhlman commented Jan 9, 2017

freeekanayaka commented Jan 9, 2017

tkuhlman commented Jan 9, 2017

		@@ -1,4 +1,5 @@
		import requests
		from urllib.parse import urlparse

Add configuration for setting the Jenkins URL. #13

Add configuration for setting the Jenkins URL. #13

Conversation

tkuhlman commented Jan 5, 2017

ryan-beisner commented Jan 5, 2017

tkuhlman commented Jan 5, 2017

freeekanayaka commented Jan 5, 2017 via email

tkuhlman commented Jan 5, 2017

freeekanayaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkuhlman commented Jan 6, 2017

freeekanayaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkuhlman commented Jan 9, 2017

tkuhlman commented Jan 9, 2017

freeekanayaka commented Jan 9, 2017

tkuhlman commented Jan 9, 2017