-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: create data connector in project #3525
base: build-project-authz-disentangle
Are you sure you want to change the base?
feat: create data connector in project #3525
Conversation
You can access the deployment of this PR at https://renku-ci-ui-3525.dev.renku.ch |
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.
The PR changes make overall sense so far 👍 Some early comments inline.
In short, there is a better way to add types from the updated APIs from other repos branches, and there is some less-than-ideal prop drilling here -- tho the components you are touching here weren't the most straightforward and your solution looks acceptable in this case.
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.
If these types are up-to-date in the renku-data-service branch feat-add-project-as-dc-owner-pt2
, instead of creating an enhanced-api
file, you should take the up-to-date spec file from there, overwrite the namespace.openapi.json
file here and run the npm command npm run generate-api:namespaceV2
.
We use the enhanced version mainly for extending the APIs -- usually transforming the response into a more convenient object (transformResponse
) or using the tags to auto-refetch content (providesTags
on GET and invalidatesTags
on the other verbs)
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.
They are not changed in the backend. But I think I will have to update them in the backend. For now I am kind of forcing the project namespace in a few places. And that is why I need this. I will remove this when it is fully integrated in the backend. I think I will be able to do this during the build.
client/src/features/projectsV2/fields/ProjectNamespaceFormField.tsx
Outdated
Show resolved
Hide resolved
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.
I'm surprised this component named "Project NamespaceFormField" is already used for Data Connectors namespaces. I was tempted to suggest creating another component, or at least renaming and moving it to make it clear it's not only for Project namespaces.
We can keep it as is for now. Still, if the logic to extract Data connector namespaces gets more complicated later in the build, we might consider separating it so that we have less code for each entity (project vs data connector).
a5d23a6
to
da6d1c7
Compare
Co-authored-by: Lorenzo Cavazzi <[email protected]>
This is the initial implementation.
Things that are missing:
/deploy renku-data-services=feat-add-project-as-dc-owner-pt2