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

feat(api): Add advanced settings endpoints to api server #1786

Merged
merged 1 commit into from
Jul 2, 2018

Conversation

btmorr
Copy link
Contributor

@btmorr btmorr commented Jun 29, 2018

overview

Add advanced settings endpoints to api server. Also added an lru_cache decorator to the "splitLabwareDefinitions" feature flag to reduce unnecessary disk IO (this is preferred anyway, as changing from old to new labware definitions without a reboot causes exceptions).

Fixes #1656

review requests

Test on robot that previously had a feature flag set to True, ensure that it remains True in new endpoint

@btmorr btmorr added feature Ticket is a feature request / PR introduces a feature api Affects the `api` project labels Jun 29, 2018
@btmorr btmorr requested review from mcous and Laura-Danielle June 29, 2018 18:57
@btmorr btmorr force-pushed the api_adv-settings-endpoints branch from 83e60c2 to 7f094e1 Compare June 29, 2018 19:10
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Looks like CI is failing, so I figured I'd put in this change request: the error response needs some tweaks to line up with existing error responses. Also, I think it's fine to pull the settings out of the error body. The app treats error responses differently from success responses, so it wouldn't know how to use settings in an error response anyway

status = 200
else:
res = _get_adv_settings()
res.update(
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, should've more properly spec'd the error response. In an error case, there's no need to respond with the settings (the app stores success response and error responses separately, so they can be different shapes).

I think 400 {"message": "ID xyz not found..."} shoudl be sufficient

@btmorr btmorr force-pushed the api_adv-settings-endpoints branch from 7f094e1 to 865f866 Compare July 2, 2018 14:38
@btmorr btmorr force-pushed the api_adv-settings-endpoints branch from 865f866 to b2f8614 Compare July 2, 2018 14:52
@codecov
Copy link

codecov bot commented Jul 2, 2018

Codecov Report

Merging #1786 into edge will decrease coverage by 0.44%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             edge   #1786      +/-   ##
=========================================
- Coverage   33.45%     33%   -0.45%     
=========================================
  Files         364     366       +2     
  Lines        5967    6090     +123     
=========================================
+ Hits         1996    2010      +14     
- Misses       3971    4080     +109
Impacted Files Coverage Δ
app-shell/lib/config.js 31.81% <0%> (-8.19%) ⬇️
components/src/lists/ListItem.js 100% <0%> (ø) ⬆️
app/src/components/TipProbe/ContinuePanel.js 0% <0%> (ø) ⬆️
app/src/pages/Robots/index.js 0% <0%> (ø) ⬆️
app/src/components/ConnectPanel/RobotList.js 0% <0%> (ø) ⬆️
app/src/components/RobotSettings/index.js 0% <0%> (ø) ⬆️
...omponents/RobotSettings/AttachedInstrumentsCard.js 0% <0%> (ø)
app/src/components/RobotSettings/InstrumentInfo.js 0% <0%> (ø)
app/src/config/index.js 50% <0%> (+4.54%) ⬆️

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 03dd305...b2f8614. Read the comment docs.

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

🍦

Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

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

🍌

@btmorr btmorr merged commit b89b4ea into edge Jul 2, 2018
@btmorr btmorr deleted the api_adv-settings-endpoints branch July 2, 2018 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project feature Ticket is a feature request / PR introduces a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants