Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

preview of proxy inclusion #112

Merged
merged 6 commits into from Oct 2, 2012
Merged

preview of proxy inclusion #112

merged 6 commits into from Oct 2, 2012

Conversation

mkrogemann
Copy link
Contributor

please review and give me feedback. I am happy to make this more complete. As it is, It helps with my personal challenge right now which is to sit behind a proxy (no authentication).
With this patch and appropriate additions to ~/.asgard/Config.groovy, such as

proxy {
host="something.com"
port=8080
}

I can now work through our proxy. By commenting the above, I can work without proxy again when I'm in a different network.

I think it would make sense to extend the initial config page and also to allow editing the config after initial configuration is done.

krogeman added 2 commits September 16, 2012 23:36
…s, such as windows proxies, passwords, etc. Intended to be reviewed by Joe Sondow
@mkrogemann
Copy link
Contributor Author

latest commit (ad7a0c3) contains a patch that fixes a brittle test. Problem with the test is that it will not work in Europe on a machine that uses a European timezone setting

@mkrogemann mkrogemann closed this Sep 17, 2012
@mkrogemann mkrogemann reopened this Sep 17, 2012
@jgritman
Copy link
Contributor

I left a few comments, but otherwise looks good.

@mkrogemann
Copy link
Contributor Author

Thanks, I will clean up as you suggested.
One thing regarding the default port though: The reason I chose the rather useless -1 as a default value in case none is given is that the Apache HttpComponents seem to follow that exact same logic in HttpHost as the javadoc suggests:

http://hc.apache.org/httpcomponents-core-ga/httpcore/apidocs/org/apache/http/HttpHost.html

krogeman added 2 commits September 18, 2012 12:02
…w US time zones but not in Europe. Now using Regular expressions. I think what is being tested here is the fact whether something is human-readable rather than the exact dates/times so I feel that the new approach is valid and less brittle
@mkrogemann
Copy link
Contributor Author

Latest commit (57a371b) contains a patch to make QueueControllerTests.groovy less brittle (does not work in European timezone).

@mkrogemann
Copy link
Contributor Author

All done as far as test failures in Europe were concerned:-)

With this latest commit (799a08d) all tests are green in Europe/Berlin. Interesting to note that all test failures stemmed from the fact that the test environment was assumed to be running in a set of selected timezones (PST, PDT, UTC)

@mkrogemann
Copy link
Contributor Author

Before the patch concerning http proxy can be merged in, the MockAmazon*Client classes need a new constructor signature. I am working on a patch...

…e a ClientConfiguration (triggered by need to have a HTTP proxy)
@mkrogemann
Copy link
Contributor Author

Here is the summary of all changes contained in this pull request and what belongs to what

Three brittle tests fixed (timezone issues), the commits are

ad7a0c3
57a371b
799a08d

Include possibility to configure an HTTP proxy (this time only via editing config file). The commits are

3a66770
3b448c7
bb39b25

@claymccoy
Copy link
Contributor

lgtm
This looks like it two separate pull requests. Thank you!
What did you have in mind for making "this more complete?" Just UI for config?

@mkrogemann
Copy link
Contributor Author

As far as making this more complete is concerned, here's what I would like to see:

  • Config form to contain proxy settings on init
  • More proxy settings such as username and password
  • A way to change config after initial setup

@joesondow
Copy link
Contributor

LGTM.

I think the additional features you have in mind could be added as separate pull requests in the future after we test and merge this one.

Changing the config after initial setup is likely to consist of wiki instructions for editing Config.groovy in a text editor. The init process for making Config.groovy is meant as a jump-starter for people to try out Asgard quickly.

@mkrogemann
Copy link
Contributor Author

I totally agree and look forward to continue with the implementation once this pull request is merged, provided it passes the quality gate first.

BTW: It would be super interesting to learn about the roadmap and maybe about features that are planned but not yet assigned.

@claymccoy
Copy link
Contributor

@mkrogemann We have plans to make information about enhancements to Asgard more transparent.

@joesondow @jgritman Since this lgte is it time to merge it?

jgritman added a commit that referenced this pull request Oct 2, 2012
preview of proxy inclusion
@jgritman jgritman merged commit 7f38a36 into Netflix:master Oct 2, 2012
@jgritman
Copy link
Contributor

jgritman commented Oct 2, 2012

Went ahead with the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants