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

feat: data connectors on the project page #3334

Merged
merged 8 commits into from
Oct 11, 2024

Conversation

ciyer
Copy link
Contributor

@ciyer ciyer commented Oct 4, 2024

/deploy renku-data-services=leafty/build-data-connectors

@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 4, 2024 07:30 — with GitHub Actions Inactive
@RenkuBot
Copy link
Contributor

RenkuBot commented Oct 4, 2024

You can access the deployment of this PR at https://renku-ci-ui-3334.dev.renku.ch

@ciyer ciyer changed the base branch from main to build/data-connectors October 4, 2024 07:45
@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 4, 2024 07:45 — with GitHub Actions Inactive
@ciyer ciyer force-pushed the build/data-connectors branch from f70273a to 21bf381 Compare October 4, 2024 07:50
@ciyer ciyer force-pushed the ciyer/project-data-connector branch from 7ddfdb6 to e70872c Compare October 4, 2024 09:58
@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 4, 2024 09:58 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 4, 2024 14:35 — with GitHub Actions Inactive
@ciyer ciyer force-pushed the ciyer/project-data-connector branch from 9173b4d to 0ade890 Compare October 8, 2024 05:46
@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 8, 2024 05:47 — with GitHub Actions Inactive
@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 8, 2024 08:51 — with GitHub Actions Inactive
@ciyer ciyer force-pushed the ciyer/project-data-connector branch from 2ff0f92 to 24ad645 Compare October 9, 2024 08:39
@ciyer ciyer force-pushed the ciyer/project-data-connector branch from 24ad645 to c55f56d Compare October 9, 2024 09:02
@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 9, 2024 09:02 — with GitHub Actions Inactive
@ciyer ciyer force-pushed the ciyer/project-data-connector branch from c55f56d to 6907969 Compare October 9, 2024 09:45
@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 9, 2024 09:46 — with GitHub Actions Inactive
@ciyer ciyer force-pushed the build/data-connectors branch 2 times, most recently from e21198e to 21bf381 Compare October 9, 2024 10:10
* refactor: move data connector api to its own folder
@ciyer ciyer force-pushed the ciyer/project-data-connector branch from 6907969 to f9acac7 Compare October 9, 2024 10:11
@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 9, 2024 10:12 — with GitHub Actions Inactive
@ciyer ciyer marked this pull request as ready for review October 9, 2024 10:12
@ciyer ciyer requested a review from a team as a code owner October 9, 2024 10:12
Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

Comments from manual testing:

  1. When creating a new data connector from a project, the visibility is "private" by default, even on a public project.
    Screenshot from 2024-10-09 13-49-51

  2. When creating a data connector from a project page, using the "advanced mode" does not seem to work. I pasted the polybox config from dev.renku.ch but then the request sent to the backend only contained type = webdav and not the other fields.

Copy link
Member

Choose a reason for hiding this comment

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

Icons live in client/src/components/icons/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4e0d010

isOpen: boolean;
onDelete: () => void;
toggleModal: () => void;
}
function DataConnectorDeleteModal({
function DataConnectorRemoveModal({
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused here. Why do we try to cram to different features inside the same component? It would be simpler imo to have <DataConnectorRemoveModal> and <DataConnectorUnlinkModal> be two separate modals. Then further down in the actions components you decide to display or not the modal and button. The code is already deciding on which text to write there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 2b4eaab

import DataConnectorActions from "./DataConnectorActions";
import useDataConnectorProjects from "./useDataConnectorProjects.hook";

function ConfigurationKeyIcon({ configKey }: { configKey: string }) {
Copy link
Member

Choose a reason for hiding this comment

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

Note: This component replaces simple one liners, e.g. <Database className={cx("bi", "me-1")} />. Therefore it does seem to increase complexity and reading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are going to remove all the icon stuff anyway, I would prefer to leave this as is in this PR and fix it in a later PR.

@ciyer ciyer temporarily deployed to renku-ci-ui-3334 October 10, 2024 13:10 — with GitHub Actions Inactive
@ciyer
Copy link
Contributor Author

ciyer commented Oct 10, 2024

Comments from manual testing:

  1. When creating a new data connector from a project, the visibility is "private" by default, even on a public project.

Good point. I have fixed this in b1c7327.

  1. When creating a data connector from a project page, using the "advanced mode" does not seem to work. I pasted the polybox config from dev.renku.ch but then the request sent to the backend only contained type = webdav and not the other fields.

I need to investigate this problem.

Copy link
Member

@leafty leafty left a comment

Choose a reason for hiding this comment

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

Great, thanks. Maybe we need to track the "advanced mode" issue.

@ciyer
Copy link
Contributor Author

ciyer commented Oct 11, 2024

Maybe we need to track the "advanced mode" issue.

I think this is the right way forward. I was able to create a data connector using advanced mode, so it does sometimes work. I have created an issue for this: #3349

@ciyer ciyer merged commit 1de9bcd into build/data-connectors Oct 11, 2024
16 of 18 checks passed
@ciyer ciyer deleted the ciyer/project-data-connector branch October 11, 2024 09:22
@RenkuBot
Copy link
Contributor

Tearing down the temporary RenkuLab deplyoment for this PR.

ciyer added a commit that referenced this pull request Oct 14, 2024
* feat: support for data connectors on user and group namespaces (#3315)
* feat: set/edit secrets for data connectors (#3330)
* feat: data connectors on the project page (#3334)
* feat: support launching sessions with data connectors (#3346)

BREAKING CHANGE: Requires renku-data-services version >= 0.xx.0
BREAKING CHANGE: Requires renku-notebooks version >= 0.xx.0

build: update to latest project api spec
ciyer added a commit that referenced this pull request Oct 16, 2024
* feat: support for data connectors on user and group namespaces (#3315)
* feat: set/edit secrets for data connectors (#3330)
* feat: data connectors on the project page (#3334)
* feat: support launching sessions with data connectors (#3346)
* minor: cosmetic improvements to data connectors (#3359)

BREAKING CHANGE: Requires renku-data-services version >= 0.xx.0
BREAKING CHANGE: Requires renku-notebooks version >= 0.xx.0
ciyer added a commit that referenced this pull request Oct 17, 2024
* feat: support for data connectors on user and group namespaces (#3315)
* feat: set/edit secrets for data connectors (#3330)
* feat: data connectors on the project page (#3334)
* feat: support launching sessions with data connectors (#3346)
* minor: cosmetic improvements to data connectors (#3359)

BREAKING CHANGE: Requires renku-data-services version >= 0.xx.0
BREAKING CHANGE: Requires renku-notebooks version >= 0.xx.0
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.

3 participants