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

[Project] Change default new_project to create instead of store #2279

Merged
merged 18 commits into from
Aug 24, 2022

Conversation

alonmr
Copy link
Member

@alonmr alonmr commented Aug 18, 2022

When creating new project allow only creation instead of update.
Add override option to delete existing project with the same name before creation.
https://jira.iguazeng.com/browse/ML-2320

@alonmr alonmr marked this pull request as ready for review August 21, 2022 13:19
@Tankilevitch Tankilevitch changed the title [Project] override existing project option on creation [Project] Change default new_project to create instead of store Aug 22, 2022
Copy link
Member

@theSaarco theSaarco left a comment

Choose a reason for hiding this comment

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

Generally looks good - should verify the deletion of project when it's non-empty (cascading deletion policy is needed).

mlrun/projects/project.py Outdated Show resolved Hide resolved
mlrun/projects/project.py Outdated Show resolved Hide resolved
tests/integration/sdk_api/projects/test_project.py Outdated Show resolved Hide resolved
Copy link
Member

@quaark quaark left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@Tankilevitch Tankilevitch 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, I think we should add a system test as well, as in integration test we are testing how it will work in the community edition (mlrun is the leader), while in Iguazio system there is a different project implementation where mlrun is the follower and iguazio is the leader.

mlrun/projects/project.py Outdated Show resolved Hide resolved
mlrun/projects/project.py Outdated Show resolved Hide resolved
tests/api/api/test_projects.py Show resolved Hide resolved
Copy link
Member

@theSaarco theSaarco left a comment

Choose a reason for hiding this comment

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

Almost there 😄 - the override flow should be tighter between deleting and creating (ideally this would be an atomic operation, but we should strive to have it as close to that as possible).

mlrun/projects/project.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Tankilevitch Tankilevitch left a comment

Choose a reason for hiding this comment

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

Looks very good, nice test coverage

@alonmr alonmr requested a review from theSaarco August 23, 2022 21:05
Copy link
Member

@theSaarco theSaarco left a comment

Choose a reason for hiding this comment

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

LGTM. Great work on the tests.

@Tankilevitch Tankilevitch merged commit 23f0e5f into mlrun:development Aug 24, 2022
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.

None yet

4 participants