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

Redshift add early startup option, rewrite to use toINI and add more options #57975

Closed
wants to merge 2 commits into from

Conversation

xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Mar 20, 2019

Motivation for this change

This adds an earlyStartup option, which allows to start redshift before the graphical interface is up.
While doing this, I realized that this can be done a lot simpler using toINI.

This is meant to solve #57961

@xaverdh xaverdh requested a review from infinisil as a code owner March 20, 2019 16:30
@xaverdh xaverdh force-pushed the redshift-early-startup branch 6 times, most recently from 50ea77f to 9666364 Compare March 20, 2019 20:27
@xaverdh xaverdh changed the title WIP: Redshift add early startup option, rewrite to use toINI Redshift add early startup option, rewrite to use toINI and add more options Mar 20, 2019
@ivan
Copy link
Member

ivan commented Sep 6, 2019

For the author, reviewers, and committers: this PR was scanned and appears to add a use of the deprecated types.string, which emits a warning as of #66346. Before merging, please change this to another type, possibly:

  • types.str for a single string where merging does not make sense, or cannot work
  • types.lines for multi-line configuration or scripts where merging is possible
  • types.listOf types.str for a mergeable list of strings

@@ -94,6 +123,15 @@ in {
'';
};

extraConfig = mkOption {
type = types.attrsOf types.string;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type = types.attrsOf types.string;
type = types.attrsOf types.lines;

@infinisil
Copy link
Member

I think we can close this for #50979, which is being worked on again and overall more finished

@infinisil infinisil closed this Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants