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

Add support for 0.5 core / 512 Mi resource requests for spring-cloud deployments #3486

Merged
merged 16 commits into from
Jun 28, 2021

Conversation

allxiao
Copy link
Member

@allxiao allxiao commented Jun 10, 2021


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
    (for the newly added code)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@yonzhan yonzhan added this to the S188 milestone Jun 10, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Jun 10, 2021

spring cloud

@zhoxing-ms zhoxing-ms modified the milestones: S188, S189 Jun 10, 2021
Comment on lines 11 to 12
if not re.match(r"^\d+m?$", cpu):
raise CLIError("CPU quantity should be millis (500m) or integer (1, 2, ...)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some explanation for the function of the regular expression?

Copy link
Member Author

Choose a reason for hiding this comment

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

The current released version only supports integer quantity ([1, 8]). The change being reviewed introduced the notation ###m for millis, and the only supported fractional amount is 500m. However, the user is also allowed to specify 1000m for 1.

The regex here is to validate the input from the client side, with some digits followed by optional m. I added some comments in the updated code.

except ValueError:
pass

if not re.match(r"^\d+[MG]i$", unified):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some explanation for the function of the regular expression?

Copy link
Member Author

Choose a reason for hiding this comment

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

We allow the user to specify the memory quantity with either ###Mi or ###Gi, for example, 1024Mi, 3Gi. The code above this line is to add backward compatibility for legacy input, which uses integer for Gi values.

The regular expression here add some client side validation for the user input, to restrict it to be some digits followed by Mi or Gi. (Note that integer input will be converted to ###Gi before this validation).

pass

if not re.match(r"^\d+[MG]i$", unified):
raise CLIError("Memory quantity should be integer followed by unit (Mi/Gi)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the specific error type InvalidArgumentValueError instead of CLIError

Copy link
Member Author

Choose a reason for hiding this comment

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

updated.

@zhoxing-ms zhoxing-ms merged commit e4a423a into Azure:main Jun 28, 2021
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