Skip to content

[WIP] Add feature to configure build plan download URL.#2024

Merged
mgsloan merged 2 commits intocommercialhaskell:masterfrom
igrep:build-plan-url-prefixes
Apr 22, 2016
Merged

[WIP] Add feature to configure build plan download URL.#2024
mgsloan merged 2 commits intocommercialhaskell:masterfrom
igrep:build-plan-url-prefixes

Conversation

@igrep
Copy link
Copy Markdown
Contributor

@igrep igrep commented Apr 11, 2016

Fixes #1794

Left TODOs

  • Test (Really necessary?)
  • Rename/Restructure the new configuration item if you can find better ones.
    • It may not have to be split into lts and nightly.
  • Documentation (I want to write after you approve).
  • ChangeLog.

Feel free to advice me on the source code structure, the new configuration item, and any others!

@mgsloan
Copy link
Copy Markdown
Contributor

mgsloan commented Apr 12, 2016

Since we may want to configure quite a few different URLs, why not just call this StackURLs? The fields can specify that they are prefixes. This would correspond to the configuration file option

urls:
    lts-prefix: ...
    nightly-prefix: ...
    ...

Also, I don't see actual usage of this in the diff, just types and commandline parsing.

@igrep
Copy link
Copy Markdown
Contributor Author

igrep commented Apr 12, 2016

Since we may want to configure quite a few different URLs, why not just call this StackURLs?

You mean I should unify the configurations like download-prefix in package-indices and latest-snapshot-url into StackURL?
Sounds good 🌟 , but I have to consider stack.yaml's compatibility doing that.
Can I make a separate pull request to make a new configuration item to unify them?
I guess the change would be very big.

Also, I don't see actual usage of this in the diff, just types and commandline parsing.

OMG! 😱 Seems I've deleted the change by mistake! Sorry, fix soon.

@igrep igrep force-pushed the build-plan-url-prefixes branch from 073fc76 to 2a26ecf Compare April 12, 2016 22:53
@igrep
Copy link
Copy Markdown
Contributor Author

igrep commented Apr 12, 2016

Fixed and rebased!

@mgsloan
Copy link
Copy Markdown
Contributor

mgsloan commented Apr 13, 2016

We can just deprecate the latest-snapshot-url field and add it to this urls list. I think the indices urls can be left where they are

@mgsloan
Copy link
Copy Markdown
Contributor

mgsloan commented Apr 22, 2016

Oops sorry for letting this languish. Hadn't seen the new commit. LGTM, thanks a bunch!

@mgsloan mgsloan merged commit 3efeb0e into commercialhaskell:master Apr 22, 2016
@TerrorJack
Copy link
Copy Markdown

Thank you for this awesome work!

mgsloan added a commit that referenced this pull request Apr 22, 2016
+ Add deprecation warning for latest-snapshot-url

fixup! Use new 'latest-snapshot' field #2024
@igrep
Copy link
Copy Markdown
Contributor Author

igrep commented Apr 22, 2016

Oh my... 😲 I was actually in progress.
I was planning to push 5874b9e ...
But, Okay! Thanks!

@mgsloan
Copy link
Copy Markdown
Contributor

mgsloan commented Apr 22, 2016

Oh, sorry, I was in PR cleanup mode

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.

3 participants