Skip to content

Deploy improvements #536

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

Merged
merged 9 commits into from
Mar 16, 2020
Merged

Deploy improvements #536

merged 9 commits into from
Mar 16, 2020

Conversation

evansiroky
Copy link
Contributor

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JSDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

This is a companion PR to ibi-group/datatools-server#282. This PR does the following:

  • Modifies the deployment UI to:
    • setting a custom AMI ID for graph builds only
    • setting a custom instance type for graph builds only
    • setting a custom region for the deployment
    • tolerate valid JSONC in the values for a custom build-config.json or router-config.json

@codecov-io
Copy link

codecov-io commented Feb 27, 2020

Codecov Report

Merging #536 into dev will decrease coverage by 28.77%.
The diff coverage is 7.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##             dev     #536       +/-   ##
==========================================
- Coverage   44.6%   15.82%   -28.78%     
==========================================
  Files        314      314               
  Lines      17114    16211      -903     
  Branches    5229     4941      -288     
==========================================
- Hits        7633     2566     -5067     
- Misses      8267    11618     +3351     
- Partials    1214     2027      +813
Flag Coverage Δ
#end_to_end_tests ?
#unit_tests 15.82% <7.27%> (-0.19%) ⬇️
Impacted Files Coverage Δ
lib/manager/util/deployment.js 61.9% <ø> (-19.05%) ⬇️
lib/admin/components/ServerSettings.js 0% <ø> (-32.49%) ⬇️
lib/manager/components/CurrentDeploymentPanel.js 17.39% <0%> (-53.45%) ⬇️
lib/manager/components/CollapsiblePanel.js 34.78% <11.11%> (-54.7%) ⬇️
lib/common/components/FormInput.js 17.07% <3.44%> (-63.58%) ⬇️
lib/manager/components/DeploymentViewer.js 17.24% <33.33%> (-48.19%) ⬇️
lib/common/util/json.js 11.76% <9.09%> (-71.57%) ⬇️
lib/manager/components/validation/TripsChart.js 0% <0%> (-80.4%) ⬇️
lib/editor/util/objects.js 14.89% <0%> (-78.73%) ⬇️
...nager/components/validation/ServicePerModeChart.js 0% <0%> (-78.27%) ⬇️
... and 265 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcfcf1c...3d16208. Read the comment docs.

@binh-dam-ibigroup binh-dam-ibigroup removed their assignment Mar 3, 2020
Copy link
Contributor

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Nice work. Good idea on the JSONC tolerance.

/**
* Check if a string is valid JSONC.
*/
export function isValidJSONC (str: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for not catching this earlier. But what is the JSONC parser supposed to permit? Can you provide a little more detail either here or in the CustomConfig component?

image

Copy link
Contributor

@landonreed landonreed Mar 4, 2020

Choose a reason for hiding this comment

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

It might be good to ref the function in OTP that parses JSON and match what is allowed there: https://github.com/opentripplanner/OpenTripPlanner/blob/27f4ed0a86157bdd4c4bc3004fec25687768d373/src/main/java/org/opentripplanner/standalone/OTPMain.java#L190-L194

So, to refine this a bit, can you allow unquoted field names also?

Copy link
Contributor

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Can you add a little more detail about the JSONC parser tolerance?

@landonreed landonreed assigned evansiroky and unassigned landonreed Mar 5, 2020
@evansiroky
Copy link
Contributor Author

Request changes added to the best of my abilities.

@evansiroky evansiroky assigned landonreed and unassigned evansiroky Mar 5, 2020
@landonreed landonreed assigned evansiroky and unassigned landonreed Mar 5, 2020
@evansiroky
Copy link
Contributor Author

More comments added.

@evansiroky evansiroky assigned landonreed and unassigned evansiroky Mar 6, 2020
@evansiroky
Copy link
Contributor Author

This PR now also allows configuration of a deployment that recreates the build image.

@landonreed landonreed merged commit a106298 into dev Mar 16, 2020
@landonreed landonreed deleted the deploy-improvements branch March 16, 2020 13:42
@landonreed
Copy link
Contributor

🎉 This PR is included in version 4.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants