-
Notifications
You must be signed in to change notification settings - Fork 276
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
Adding an https-url import definition example to import a large impor… #1010
Conversation
…t definition at boot time from the web
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the NodePort
section. Otherwise, it's great.
…t definition at boot time from the web
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments about resource limit and request and the memory high water mark configurations. If they are not required for the example, let's remove it. If they are indeed required, may I suggest adding comments in the example or the README to explain what they are for? 😃
requests: | ||
cpu: 2 | ||
memory: 8Gi | ||
limits: | ||
cpu: 2 | ||
memory: 8Gi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the resource request and limit required for this example to work? If not, lets remove it to keep the example simple and just about the importing definition itself.
…t definition at boot time from the web
@ChunyiLyu thanks for your review, indeed I was planning to write the motivation in the README but I then forgot. The example takes a lot of memory to load a very big definition file, so I think better to put the resource requests/limits. I removed the vm_memory_high_watermark_paging_ratio as it wasn't really necessary for the example and I updated the README file. |
…t definition at boot time from the web
…t definition at boot time from the web
This closes #
Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Summary Of Changes
Additional Context
Local Testing
Please ensure you run the unit, integration and system tests before approving the PR.
To run the unit and integration tests:
You will need to target a k8s cluster and have the operator deployed for running the system tests.
For example, for a Kubernetes context named
dev-bunny
: