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

Use latest nancy #9

Merged
merged 28 commits into from Feb 19, 2021
Merged

Use latest nancy #9

merged 28 commits into from Feb 19, 2021

Conversation

bhamail
Copy link
Contributor

@bhamail bhamail commented Feb 17, 2021

This PR adds a new configuration option: nancyVersion.
Valid values for this option are 'latest' or a specific version, like `v.1.0.6'.
Default value is 'latest'.

I chose to use a call to the github api to determine the latest version for a few reasons:

  • I have bad memories of "un-versioned" jar names for the days of java/maven
  • I like being able to easily "log" the download url showing exactly which version we're using.

Fixes issue #3

Comment on lines +21 to +24
RUN apk add --no-cache curl

# required to get grep that supports -P option
RUN apk add --no-cache --upgrade grep
Copy link
Member

Choose a reason for hiding this comment

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

Not a major thing, but I think a good idea in general is to clean up after yourself, so maybe delete these packages unless they are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the upgraded grep must stick around because it is used later when running the docker image (to detect the "latest" version).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that makes sense. I'd ditch curl maybe unless you need it? It's not a huge deal but I personally love leaving the environment as pristine as possible.

install-nancy.sh Outdated Show resolved Hide resolved
Copy link
Member

@DarthHater DarthHater left a comment

Choose a reason for hiding this comment

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

No major feedback, left a couple comments. You know how this works better than I do at this point!

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

2 participants