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

[tools/shoestring]: light rest implementation #1165

Open
wants to merge 30 commits into
base: dev
Choose a base branch
from

Conversation

AnthonyLaw
Copy link
Member

@AnthonyLaw AnthonyLaw commented Nov 8, 2024

What was the issue?

  • Implement light REST functionality into tools.

What's the fix?

  • to set full API node in sai.ini config: features = API and lightApi = false.
  • to set light rest node in sai.ini config: features = API and lightApi = true.
  • Added full_api flag in node configuration to define setup full API node.
  • Added a docker compose template for light setup.
  • Implemtation all related light config in a shoestring.

Light rest is a subset of full API. I extracted the configuration and applied it to the condition check.

@AnthonyLaw AnthonyLaw force-pushed the tools/shoestrig-light-rest branch from da06998 to 4b88efe Compare November 8, 2024 19:43
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.23%. Comparing base (4afb34a) to head (ee91545).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1165      +/-   ##
==========================================
- Coverage   98.25%   98.23%   -0.02%     
==========================================
  Files         162      157       -5     
  Lines        6630     6501     -129     
  Branches      143      143              
==========================================
- Hits         6514     6386     -128     
+ Misses        116      115       -1     
Flag Coverage Δ
explorer-rest ?
tools-shoestring 96.87% <100.00%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
tools/shoestring/shoestring/commands/reset_data.py 100.00% <100.00%> (ø)
tools/shoestring/shoestring/commands/setup.py 100.00% <ø> (ø)
tools/shoestring/shoestring/commands/upgrade.py 100.00% <100.00%> (ø)
...s/shoestring/shoestring/healthagents/websockets.py 100.00% <100.00%> (ø)
tools/shoestring/shoestring/internal/Preparer.py 100.00% <100.00%> (ø)
...ing/shoestring/internal/ShoestringConfiguration.py 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

@AnthonyLaw AnthonyLaw force-pushed the tools/shoestrig-light-rest branch from 4829dd9 to 535ba7b Compare November 14, 2024 13:50
@AnthonyLaw AnthonyLaw marked this pull request as ready for review November 15, 2024 05:04
@AnthonyLaw AnthonyLaw force-pushed the tools/shoestrig-light-rest branch from 535ba7b to 202a386 Compare November 15, 2024 05:06
@AnthonyLaw AnthonyLaw requested review from Jaguar0625, gimre-xymcity and Wayonb and removed request for gimre-xymcity November 15, 2024 13:04
@AnthonyLaw AnthonyLaw force-pushed the tools/shoestrig-light-rest branch 3 times, most recently from a3c3020 to 59aae1a Compare November 22, 2024 18:59
@AnthonyLaw AnthonyLaw requested a review from Wayonb November 22, 2024 19:26
Copy link
Contributor

@Wayonb Wayonb left a comment

Choose a reason for hiding this comment

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

looks good 👍🏾

have a few questions.

tools/shoestring/shoestring/healthagents/rest_api.py Outdated Show resolved Hide resolved
tools/shoestring/shoestring/internal/Preparer.py Outdated Show resolved Hide resolved
@AnthonyLaw AnthonyLaw requested a review from Wayonb November 27, 2024 09:31
@AnthonyLaw AnthonyLaw force-pushed the tools/shoestrig-light-rest branch from f416269 to d13e8b9 Compare December 2, 2024 03:42
@AnthonyLaw
Copy link
Member Author

I guess now that you added back the rest_cache folder then this is not needed? 🤔

I must understand wrongly, I thought you wanted to keep the rest_cache.
I will revert the change.

@Wayonb
Copy link
Contributor

Wayonb commented Dec 4, 2024

I guess now that you added back the rest_cache folder then this is not needed? 🤔

I must understand wrongly, I thought you wanted to keep the rest_cache. I will revert the change.

No, I meant since you added back the rest_cache, the below code can be moved back

		if NodeFeatures.API in self.config.node.features and self.config.node.api_https:
			directories.append(self.directories.https_proxy)

		if NodeFeatures.API in self.config.node.features:
			directories.append(self.directories.rest_cache)

=>

		if NodeFeatures.API in self.config.node.features:
			directories.append(self.directories.rest_cache)
			 if self.config.node.api_https:
			     directories.append(self.directories.https_proxy)
"

@AnthonyLaw AnthonyLaw requested a review from Wayonb December 5, 2024 16:58
Copy link
Contributor

@Wayonb Wayonb left a comment

Choose a reason for hiding this comment

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

👍🏾

@AnthonyLaw AnthonyLaw force-pushed the tools/shoestrig-light-rest branch from decac18 to ee91545 Compare December 6, 2024 03:41
@AnthonyLaw AnthonyLaw force-pushed the tools/shoestrig-light-rest branch from ee91545 to 08690a2 Compare December 16, 2024 15:27
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.

3 participants