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

Closes #220 Latest stable is now the default channel #221

Merged
merged 2 commits into from Nov 4, 2020
Merged

Closes #220 Latest stable is now the default channel #221

merged 2 commits into from Nov 4, 2020

Conversation

plfaucher
Copy link
Contributor

  • Declared a variable latest_stable pointing to the latest stable channel near channels definition. (Line 293)
  • Used the variable on line 124 for defaultChannel when unspecified.

This is imho better than only changing the line Maybe.withDefault "unstable" to Maybe.withDefault "20.09", because if done this way we would have to change version numbers at two places (Line 124 and the definitions around Line 280)

src/Search.elm Outdated
Comment on lines 290 to 294
-- Concentrates logic near releases definition.
-- Otherwise you would have remember to modify @:124 defaultChannel each time.
-- Bump when new channel is released.
latest_stable : String
latest_stable = "20.09"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should do this more programmatically. It would be more work but also require less maintenance.

src/Search.elm Outdated
-- Otherwise you would have remember to modify @:124 defaultChannel each time.
-- Bump when new channel is released.
latest_stable : String
latest_stable = "20.09"
Copy link
Member

Choose a reason for hiding this comment

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

lets rename this into defaultChannel, but it would be actually nice to change how we handle channels in a way that we wouldn't have to update this every time the release is made.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. Though I'm not against merging this PR as first step and then continuing with more future proofed solution in new PR.

Copy link
Member

Choose a reason for hiding this comment

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

We could fetch active channels via the metrics, like status.nixos.org already does it (https://github.com/NixOS/nixos-org-configurations/blob/master/delft/eris/status-page/status.js#L102) and then pass this list of string down (flake.nix -> src/index.js -> src/Search.elm)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaultChannel is already defined on line 121 for the request model unfortunately.

You are both 100% right that it should not be a manual update in the code. I view default channel issue and the automatic channel fetching as two separates enhancements, so if you agree I would:

  1. Create a separate issue for the channel fetching. {Someone} can then work on it. I am very much a novice in Elm but I wouldn't mind trying :)
  2. Rename latest_stable to defaultChannelStable or anything else to your liking that will not conflict with existing code.

Copy link
Member

Choose a reason for hiding this comment

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

I renamed the latest_stable to defaultChannel and merge the PR since it is a step forward.

If you would like to work on this I can offer help to mentor you. The task will require a bit more then just Elm. Here is how things should be done (in order):

  • create ./update.sh where we will fetch active channels (like status.nixos.org) and store then in ./channels.json
  • add update step to cron workflow (.github/workflows/cron.yml). we also need to commit back the changes like we do in nixos-homepage
  • in default.nix read channels.json and pass make it available in the dev/build environment via variable like we already do this for ELASTICSEARCH_MAPPING_SCHEMA_VERSION.
  • in src/index.js we would use just passed variable as a initialization flag, like we already do for ELASTICSEARCH_MAPPING_SCHEMA_VERSION.
  • we would extend Flags alias type in src/Main.elm and pass it down to src/Search.elm

@garbas garbas merged commit 1407641 into NixOS:master Nov 4, 2020
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

3 participants