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

[Nuctl][Dashboard] Move autofix functionality to platform side #3122

Merged
merged 13 commits into from
Jan 23, 2024

Conversation

rokatyy
Copy link
Contributor

@rokatyy rokatyy commented Jan 15, 2024

jira - https://jira.iguazeng.com/browse/NUC-127

This pull request moves the autofix functionality to the platform side to achieve the following:

  1. Avoid going through the entire import flow twice.
  2. Allow passing 'autofix' as a header within the API request, enabling users to import with the autofix option from the UI.
  3. Consolidate validation and autofix functionality in one place, but still following single responsibility principle and not touching validation code directly

Currenntly maxRetries is equal to 1, because we support only one error to be autofixed.

@rokatyy rokatyy changed the title [Nuctl][Dashboard] Moved autofix functionality to validation side [Nuctl][Dashboard] Move autofix functionality to platform side Jan 15, 2024
@rokatyy rokatyy marked this pull request as ready for review January 15, 2024 16:14
Copy link
Contributor

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

I am not yet convinced doing the auto-fixing while validating is the right way to do it.
While I understand this saves us another deployment/validation cycle, this pretty much defies the SOLID principle of single responsibility.

Changing the function configuration on-the-fly is very intrusive and can cause some unexpected behaviors from users if not used properly.
We wanted this feature in the upgrade so it wouldn't fail, but generally I prefer that the deployment will fail if the wrong configuration is given.

I lean towards seperating the fixing from nuctl to a "Fixer" kind of object in the functionconfig package, that can be called from the anywhere, but mainly used when importing functions.

pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/nuctl/command/import.go Outdated Show resolved Hide resolved
@rokatyy
Copy link
Contributor Author

rokatyy commented Jan 15, 2024

@TomerShor agreed with those points, let's discuss tomorrow on nuclio sync

@rokatyy rokatyy marked this pull request as draft January 16, 2024 16:21
pkg/nuctl/command/import.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Show resolved Hide resolved
pkg/platform/kube/platform.go Outdated Show resolved Hide resolved
pkg/platform/local/platform.go Outdated Show resolved Hide resolved
pkg/platform/local/platform.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
Copy link
Contributor

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

Last comment from me - looking great

pkg/platform/abstract/platform.go Outdated Show resolved Hide resolved
@rokatyy rokatyy marked this pull request as ready for review January 22, 2024 08:21
Copy link
Contributor

@TomerShor TomerShor left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@TomerShor TomerShor merged commit 95f8af2 into nuclio:development Jan 23, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants