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

Update CLI command to support config from config/content-security-policy.js and fall back to legacy config/environment.js config. #103

Conversation

jelhan
Copy link
Collaborator

@jelhan jelhan commented Jul 25, 2019

Closes #98

lib/commands.js Outdated Show resolved Hide resolved
@jelhan jelhan force-pushed the issue-98-cli-command-is-not-using-new-configuration branch from 91975fc to f393e84 Compare August 6, 2019 10:58
@jelhan
Copy link
Collaborator Author

jelhan commented Aug 6, 2019

Rebased on top of #104. Tests should pass now. Still a WIP as #104 needs to be merged before. Will introduce merge conflicts with #105. I don't care which one is merged before.

@jelhan jelhan force-pushed the issue-98-cli-command-is-not-using-new-configuration branch from c7c275c to 05a5a79 Compare August 7, 2019 19:24
@jelhan
Copy link
Collaborator Author

jelhan commented Aug 7, 2019

Rebased on top of #105. Should be ready as soon as that one is merged.

@jelhan jelhan force-pushed the issue-98-cli-command-is-not-using-new-configuration branch from 05a5a79 to 9168668 Compare August 8, 2019 14:34
@jelhan jelhan changed the title WIP: CLI command should respect new configuration CLI command should respect new configuration Aug 8, 2019
@jelhan
Copy link
Collaborator Author

jelhan commented Aug 8, 2019

Rebased on top of master. @rwjblue ready to be merged from my side.

@jelhan jelhan force-pushed the issue-98-cli-command-is-not-using-new-configuration branch from 9168668 to d9ddc5e Compare August 8, 2019 18:22
@jelhan
Copy link
Collaborator Author

jelhan commented Aug 8, 2019

Rewritten commit history so that #91 is not blocked anymore by this one but shares a commit with it.

@rwjblue
Copy link
Owner

rwjblue commented Aug 9, 2019

Sorry @jelhan, I think this will need a rebase (on top of #107). Other than that, this looks good to me...

@jelhan jelhan force-pushed the issue-98-cli-command-is-not-using-new-configuration branch from d9ddc5e to 4c2606a Compare August 13, 2019 09:41
@jelhan
Copy link
Collaborator Author

jelhan commented Aug 13, 2019

@rwjblue Rebased. Not sure what happened with Travis CI. Seems like it created five builds. Four seem to be stale. But should be safe to merge cause one passed.

@jelhan jelhan mentioned this pull request Aug 13, 2019
10 tasks
readConfig
} = require('./utils');

const CSP_HEADER = 'Content-Security-Policy';
Copy link
Owner

Choose a reason for hiding this comment

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

Definitely non-blocking, and probably just a bit too nitpicky but in general I prefer to avoid the equals alignment stuff. It makes future changes change lines that are unrelated (e.g. when adding new variables with a slightly longer name), and then makes it just a tiny bit harder to follow whats going on through the git history.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

💯 I was just adopting the existing coding style.

@rwjblue rwjblue merged commit 279870d into rwjblue:master Aug 13, 2019
@rwjblue rwjblue changed the title CLI command should respect new configuration Update CLI command to support config from config/content-security-policy.js and fall back to legacy config/environment.js config. Aug 13, 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.

CLI command is not using refactored configuration
2 participants