-
Notifications
You must be signed in to change notification settings - Fork 8
Setup tunning #15
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
Setup tunning #15
Conversation
bin/install-checkstyle.sh
Outdated
@@ -0,0 +1,6 @@ | |||
#!/bin/sh | |||
|
|||
curl -s -i https://sourceforge.net/projects/checkstyle/files/latest/download | \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to just install the latest? I think there's value in being explicit about which version we're using, so that it's easy to determine by reading the code, and so we know exactly what to change to perform an upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxjacobson I wanted an automated way to upgrade, but maybe this is not the pattern we are following in the other engines and I see your point.
What about turning this into a 2-step thing? Instead of calling this directly from Dockerfile
we would make upgrade
first, then make image
would have the latest...
...hmm, then we would have to commit those files, not sure I like this idea anymore.
Probably simpler just being explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I could have a make
target just to show the latest URL. Then use it explicitly.
…plicit version set
bin/install-checkstyle.sh
Outdated
#!/bin/sh | ||
|
||
# use `make upgrade` to update this URL to the latest version | ||
URL='https://downloads.sourceforge.net/project/checkstyle/checkstyle/8.2/checkstyle-8.2-all.jar?r=&ts=1505330021&use_mirror=razaoinfo' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxjacobson WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the deal with the query string? Can we leave it off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxjacobson Seems to be how it selects the mirror. Testing without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxjacobson It works. Updated to take it off.
@maxjacobson Did the same thing ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, lgtm
Uh oh!
There was an error while loading. Please reload this page.