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

Simplify folder controller schema, change folder uid if needed #1039

Merged
merged 10 commits into from May 15, 2023

Conversation

weisdd
Copy link
Collaborator

@weisdd weisdd commented May 7, 2023

Several improvements for folder controller:

  • I think there isn't much need in offering raw json for folders. - Their API is very limited (docs), we can simplify operator's code (no need to care about errors in json + no need to decode it) and UX a bit by exposing custom title in a separate field.
  • Requeue folder sync in case of errors;
  • Reorganize onFolderCreated a bit to make room for further introduction of permissions (those will be in json);
  • Replace folder uid if needed (e.g. folder was created outside of operator or through dashboard controller => exists with different uid).

@weisdd weisdd requested review from pb82, NissesSenap and HVBE May 7, 2023 15:45
@weisdd weisdd self-assigned this May 7, 2023
@weisdd weisdd changed the title Simplify folder controller schema Simplify folder controller schema, change folder uid if needed May 7, 2023
@NissesSenap
Copy link
Collaborator

I will take a deeper look tomorrow. I think we also should update some docs to match the changes. Or didn't we add any folder example?

@weisdd
Copy link
Collaborator Author

weisdd commented May 7, 2023

@NissesSenap unless I overlooked them, we don't have any examples with folders yet. I thought of adding one after permissions are implemented. For basic permissions (just raw json), I have some code already, it's just a bit of extra code on top of the changes from this PR, so I'll park another PR till this one is discussed/reviewed :)

@pb82
Copy link
Collaborator

pb82 commented May 14, 2023

@weisdd verified and works, folders get created. Manifests need to be udpated though (make generate, make manifests & make bundle).

@NissesSenap
Copy link
Collaborator

@pb82 if we want to enforce that we should do it through CI. We let's add that in a separate PR and we can fix all the generation there.

@weisdd
Copy link
Collaborator Author

weisdd commented May 15, 2023

@pb82 Seems like bundle is not part of test in Makefile, we should add it.
Ran make bundle manually, everything should be in sync now.

@weisdd weisdd force-pushed the fix/folder-controller-schema branch from c91924c to 27c1833 Compare May 15, 2023 09:23
@pb82
Copy link
Collaborator

pb82 commented May 15, 2023

Thanks @weisdd ! @NissesSenap yes, it would be good to make this part of the pipeline.

@pb82 pb82 merged commit 0694163 into master May 15, 2023
9 checks passed
@pb82 pb82 deleted the fix/folder-controller-schema branch May 15, 2023 09:34
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

3 participants