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

Lazy-load server data with ec2 instance info #615

Merged
merged 2 commits into from
Oct 9, 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 PR changes the way data is loaded for the ServerSettings admin component. Whenever a panel showing the details about a server is expanded, the UI will now make a call to a new endpoint in datatools-server to load the full information about the server which could include data about ec2 instances. This is done to avoid making unnecessary calls to AWS until that data is actually needed.

This PR requires using the code in ibi-group/datatools-server#343.

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2020

Codecov Report

Merging #615 into dev will decrease coverage by 27.79%.
The diff coverage is 5.55%.

Impacted file tree graph

@@             Coverage Diff             @@
##              dev     #615       +/-   ##
===========================================
- Coverage   43.45%   15.66%   -27.80%     
===========================================
  Files         323      323               
  Lines       17589    16439     -1150     
  Branches     5363     4992      -371     
===========================================
- Hits         7644     2575     -5069     
- Misses       8670    11845     +3175     
- Partials     1275     2019      +744     
Flag Coverage Δ
#end_to_end_tests ?
#unit_tests 15.66% <5.55%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/admin/components/ServerSettings.js 0.00% <0.00%> (-32.49%) ⬇️
lib/admin/reducers/servers.js 37.50% <0.00%> (-27.21%) ⬇️
lib/manager/components/CollapsiblePanel.js 33.33% <0.00%> (-58.67%) ⬇️
lib/admin/actions/admin.js 18.55% <14.28%> (-40.79%) ⬇️
...nager/components/validation/ServicePerModeChart.js 0.00% <0.00%> (-85.90%) ⬇️
lib/manager/components/validation/TripsChart.js 0.00% <0.00%> (-80.40%) ⬇️
lib/common/util/map-keys.js 25.00% <0.00%> (-75.00%) ⬇️
lib/manager/components/HomeProjectDropdown.js 0.00% <0.00%> (-72.73%) ⬇️
lib/editor/util/objects.js 14.89% <0.00%> (-72.35%) ⬇️
lib/editor/components/map/TextPath.js 0.00% <0.00%> (-70.00%) ⬇️
... and 270 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 71eb526...9e02775. Read the comment docs.

)
return update(state, {
isFetching: { $set: false },
data: { [serverIdx]: { $set: action.payload } }
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily needed, but seems like an opportunity to reuse the serverData variable?

_onExpandServer = (index: number) => {
this.props.fetchServer(this.state.otpServers[index].id)
}

_onSaveServer = (index: number) => this.props.updateServer(this.state.otpServers[index])
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if a server is expanded, you apply updates, and click save? Are the server ec2 instances no longer shown?

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.

One minor comment, one that probably needs addressing.

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

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Subject to @landonreed's comment about refetching EC2 instances on an expanded server panel.

@binh-dam-ibigroup binh-dam-ibigroup removed their assignment Oct 6, 2020
@evansiroky
Copy link
Contributor Author

Good catch. I just pushed a change that addresses this.

@evansiroky evansiroky assigned landonreed and unassigned evansiroky Oct 6, 2020
@landonreed landonreed assigned evansiroky and unassigned landonreed Oct 8, 2020
@evansiroky evansiroky merged commit 3e1b15a into dev Oct 9, 2020
@landonreed landonreed deleted the lazy-load-server-ec2-instance-data branch October 21, 2020 16:41
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.

4 participants