-
Notifications
You must be signed in to change notification settings - Fork 81
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 mlstudio integ tests #1535
add mlstudio integ tests #1535
Conversation
if errors := r.json().get('errors'): | ||
raise GqlError(errors) | ||
r.raise_for_status() |
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.
Irrelevant change to first check for any errors and then raise for status. Sometimes the returned code when there are errors is not 2xx.
|
||
@pytest.fixture(scope='session') | ||
def smstudio_user1(session_id, client1, persistent_env1): | ||
env_uri = persistent_env1.environmentUri |
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.
Don't we have to ensure the create_environment
API call has {'key': 'mlstudioEnabled', 'value': 'true'},
as part of parameters?
Otherwise we wouldn't be able to spin up mew sm studio users
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.
yes you are right, I enabled it manually on my persistent environment and forgot to add it in code
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.
Left a comment around creating env ensuring ML Studio is enabled.
Ideally we only enable on the 1 persistent env and not on the session envs so we do not need to add additional wait time for create SM Studio Domains for each env which would be lengthy
Otherwise code looks good - will try out the tests in meantime
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.
looks good and tests passing on my aws deployment too - approved
Feature or Bugfix
Feature
Detail
Adding integration tests for ML Studio
PENDING TESTS PASSING IN DEV AWS ENV
Relates
related to #1220 and resolves #1534
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.