(PDB-1068) Recognize prefix urls in import export by rbrw · Pull Request #1206 · puppetlabs/puppetdb

rbrw · 2015-01-08T17:33:43Z

Note that there are 4 commits, which might or might not be easier to review individually, and I wondered about defaulting the base-url :protocol to "http". Right now it has to be specified every time.

(This series is based on the PDB-1070 PR and so may need to be rebased once that goes in.)

pljenkinsro · 2015-01-08T18:31:21Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/platform_puppetdb_intn-sys_pr/385/

senior · 2015-01-09T16:19:59Z

@rbrw Now that @ajroetker PR has been merged, this needs a rebase

pljenkinsro · 2015-01-09T18:01:24Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/platform_puppetdb_intn-sys_pr/393/

wkalt · 2015-01-09T18:01:52Z

@pljenkinsro retest this please

rbrw · 2015-01-09T18:07:52Z

Rebased to master.

pljenkinsro · 2015-01-09T19:01:21Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/platform_puppetdb_intn-sys_pr/394/

senior · 2015-01-12T12:55:16Z

src/puppetlabs/puppetdb/cli/export.clj

I think exit-process should be something different from :type. We usually use type to indicate the type of error that occurred, rather than what do do about the error. So in this case maybe invalid-source is the error type? I also prefer the namespace keywords in these potentially global maps, so we know where it came from, i.e. ::invalid-source.

senior · 2015-01-12T15:31:46Z

test/puppetlabs/puppetdb/cli/import_export_roundtrip_test.clj

I think this would simplify the code here.

senior · 2015-01-12T16:37:09Z

This is a good PR, I really like the throw/exit pattern by the way, I think it's better than the thunk passing that I did in the services ns. At first I wasn't sure about the base-url map stuff, but after reading through more of the change, I think it's an improvement over just a bare URL string. We have some utility functions built up around it and it allows us to give better, more specific error messages.

rbrw · 2015-01-13T23:50:24Z

OK, thanks for all the suggestions. Hopefully the push below address the issues. It also adds some minor additional tests, and a hopefully better wrapper for import/export main.

I've rebased the previous push as ticket/master/pdb-1068-recognize-prefix-urls-in-import-export-prev so that you should be able to get a pretty good sense of the changes via

git diff rbrw/ticket/master/pdb-1068-recognize-prefix-urls-in-import-export-prev..rbrw/ticket/master/pdb-1068-recognize-prefix-urls-in-import-export

if you fetch both branches. The previous push itself is also still available as

rbrw/ticket/master/pdb-1068-recognize-prefix-urls-in-import-export-v1

in case the "prev" rebase did something unexpected.

Thanks

pljenkinsro · 2015-01-14T00:16:03Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/platform_puppetdb_intn-sys_pr/412/

senior · 2015-01-14T13:54:24Z

Error messages are much better with this patch, before:

$ lein run export -o the-file.tar.gz --host foo:bar
java.lang.NumberFormatException: For input string: "bar:8080"

after:

lein run export -o the-file.tar.gz --host foo:bar
clojure.lang.ExceptionInfo: Invalid source (Malformed IPv6 address at index 8: http://[foo:bar]:8080//v4)

I'm looking into it further, not sure why there's a double slash there if a url-prefix isn't specified.

To accomplish that, change all the relevant functions to accept a new "base-url" argument which is just a map that must contain :protocol, :host, :port, and optional :version and :prefix keys. If the :version isn't set, default it to :v4. When creating URL strings for client/get, Use a "base-url -> URL -> URI -> ASCII" trip to check for problems and perform escaping. Adjust the jetty utils to manage a *base-url* rather than just a *port*, so that tests can interact with the :prefix, etc.

To do so, add a main wrapper that allows us to throw a :utils/exit-process exception instead of calling System/exit directly. Have the wrapper handle other generally useful administrivia, i.e. calling shutdown-agents, flushing output, etc. Eventually we might want to extend this approach further, both within import/export, and elsewhere. Aside from making testing easier, it also allows any pending exception handlers to fire.

pljenkinsro · 2015-01-14T14:55:53Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/platform_puppetdb_intn-sys_pr/413/

rbrw · 2015-01-14T14:58:20Z

The double slash was caused by not checking for empty? in base-url->str, and I've fixed the prn.

This series should fix both issues, and the previous series should be available as https://github.com/rbrw/puppetdb/tree/ticket/master/pdb-1068-recognize-prefix-urls-in-import-export-v2

rbrw · 2015-01-14T15:14:00Z

@pljenkinsro retest this please

pljenkinsro · 2015-01-14T16:13:10Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/platform_puppetdb_intn-sys_pr/414/

ajroetker · 2015-01-14T16:45:22Z

src/puppetlabs/puppetdb/cli/export.clj

So I know in config files in PE when we got to the point of having to specify all these things, we decided to just use a single base-url item that the user specifies (e.g. http://localhost:8080/my_prefix) instead, parsing it into the map when we received it. Might make things cleaner!

rbrw · 2015-01-14T22:50:00Z

@pljenkinsro retest this please

rbrw · 2015-01-14T22:50:39Z

The failures seem likely to be transient.

pljenkinsro · 2015-01-14T23:49:24Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/platform_puppetdb_intn-sys_pr/419/

senior · 2015-01-15T00:17:56Z

@pljenkinsro retest this please

pljenkinsro · 2015-01-15T01:59:11Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.puppetlabs.com/job/platform_puppetdb_intn-sys_pr/423/

…refix-urls-in-import-export (PDB-1068) Recognize prefix urls in import export

rbrw changed the title ~~Ticket/master/pdb 1068 recognize prefix urls in import export~~ (PDB-1068) Recognize prefix urls in import export Jan 8, 2015

senior added the work in progress (...and please don't merge) label Jan 9, 2015

rbrw force-pushed the ticket/master/pdb-1068-recognize-prefix-urls-in-import-export branch from 1a40a44 to e03a27d Compare January 9, 2015 17:56

rbrw removed the work in progress (...and please don't merge) label Jan 9, 2015

senior reviewed Jan 12, 2015
View reviewed changes

kbarber added the work in progress (...and please don't merge) label Jan 12, 2015

senior reviewed Jan 12, 2015
View reviewed changes

test/puppetlabs/puppetdb/cli/import_export_roundtrip_test.clj Outdated

Copy link

Contributor

senior Jan 12, 2015

Choose a reason for hiding this comment

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

I think this would simplify the code here.

rbrw added 2 commits January 12, 2015 16:04

Use defn-validated in preparation for base-url

ee9a4a2

export: centralize the the api-version setting

f67713e

rbrw force-pushed the ticket/master/pdb-1068-recognize-prefix-urls-in-import-export branch from e03a27d to 9567481 Compare January 13, 2015 23:46

rbrw added 2 commits January 14, 2015 08:45

(PDB-1068) Support import/export --url-prefix

0a24ac8

rbrw force-pushed the ticket/master/pdb-1068-recognize-prefix-urls-in-import-export branch from 811e7bc to 0a24ac8 Compare January 14, 2015 14:53

ajroetker reviewed Jan 14, 2015
View reviewed changes

senior added a commit that referenced this pull request Jan 15, 2015

Merge pull request #1206 from rbrw/ticket/master/pdb-1068-recognize-p…

6a34398

…refix-urls-in-import-export (PDB-1068) Recognize prefix urls in import export

senior merged commit 6a34398 into puppetlabs:master Jan 15, 2015

Conversation

rbrw commented Jan 8, 2015

Uh oh!

pljenkinsro commented Jan 8, 2015

Uh oh!

senior commented Jan 9, 2015

Uh oh!

pljenkinsro commented Jan 9, 2015

Uh oh!

wkalt commented Jan 9, 2015

Uh oh!

rbrw commented Jan 9, 2015

Uh oh!

pljenkinsro commented Jan 9, 2015

Uh oh!

senior Jan 12, 2015

Choose a reason for hiding this comment

Uh oh!

senior Jan 12, 2015

Choose a reason for hiding this comment

Uh oh!

senior commented Jan 12, 2015

Uh oh!

rbrw commented Jan 13, 2015

Uh oh!

pljenkinsro commented Jan 14, 2015

Uh oh!

senior commented Jan 14, 2015

Uh oh!

pljenkinsro commented Jan 14, 2015

Uh oh!

rbrw commented Jan 14, 2015

Uh oh!

rbrw commented Jan 14, 2015

Uh oh!

pljenkinsro commented Jan 14, 2015

Uh oh!

ajroetker Jan 14, 2015

Choose a reason for hiding this comment

Uh oh!

rbrw commented Jan 14, 2015

Uh oh!

rbrw commented Jan 14, 2015

Uh oh!

pljenkinsro commented Jan 14, 2015

Uh oh!

senior commented Jan 15, 2015

Uh oh!

pljenkinsro commented Jan 15, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants