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

Remove obsolete read_entry_data method from class ComponentCatalogConnector #3141

Merged
merged 2 commits into from
Dec 30, 2024

Conversation

ptitzler
Copy link
Member

@ptitzler ptitzler commented Mar 30, 2023

Closes #2514

What changes were proposed in this pull request?

How was this pull request tested?

  1. Installed https://github.com/ptitzler/examples/tree/main/component-catalog-connectors/kfp-example-components-connector, which only implements the obsolete method. (elyra-examples-kfp-catalog==0.1.0)
  2. Created an instance of that catalog connector in Elyra.
  3. Confirmed that the expected error is logged.

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@ptitzler ptitzler added component:catalog connectors Access to component catalogs impact:breaking change Delivery introduces a change that is not backward compatible labels Mar 30, 2023
@ptitzler ptitzler added this to the 4.0.0 milestone Mar 30, 2023
@ptitzler ptitzler added the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Mar 30, 2023
Signed-off-by: Patrick Titzler <[email protected]>
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

I didn't take these changes for a spin, but they look good. Thanks @ptitzler.

@lresende lresende mentioned this pull request Dec 21, 2024
7 tasks
@shalberd shalberd requested a review from kevin-bates December 26, 2024 17:02
@shalberd
Copy link
Contributor

shalberd commented Dec 26, 2024

Hello @lresende @caponetto

There is some note in the code of Elyra 4 here ... but I am unsure conceptually what Patrick wanted to achieve back then.
Has no impact on my generic node processor / Airflow generic template code modifications for Airflow 2, as far as I can see.

Just seeing this is part of the release 4.0 tracker ... just trying to understand what this code here was :-)
"example component connector" for airflow and kfp sounds unfamiliar.

@lresende
Copy link
Member

@shalberd see more details in #2514

@lresende lresende removed the status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. label Dec 29, 2024
@lresende lresende merged commit a60ae72 into elyra-ai:main Dec 30, 2024
@caponetto
Copy link
Contributor

@lresende looks like this PR has broken the CI.
https://github.com/elyra-ai/elyra/actions/runs/12544953702

lresende added a commit to lresende/elyra that referenced this pull request Dec 31, 2024
…talogConnector (elyra-ai#3141)"

This reverts commit a60ae72.
This is breaking the build as it seems its not complete yet.
lresende added a commit to lresende/elyra that referenced this pull request Dec 31, 2024
…talogConnector (elyra-ai#3141)"

This reverts commit a60ae72.
This is breaking the build as it seems its not complete yet.

Signed-off-by: Luciano Resende <[email protected]>
lresende added a commit to lresende/elyra that referenced this pull request Dec 31, 2024
…talogConnector (elyra-ai#3141)"

This reverts commit a60ae72.
This is breaking the build as it seems its not complete yet.

Signed-off-by: Luciano Resende <[email protected]>
lresende added a commit to lresende/elyra that referenced this pull request Dec 31, 2024
…talogConnector (elyra-ai#3141)"

This reverts commit a60ae72.
This is breaking the build as it seems its not complete yet.

Signed-off-by: Luciano Resende <[email protected]>
@lresende
Copy link
Member

@lresende looks like this PR has broken the CI. https://github.com/elyra-ai/elyra/actions/runs/12544953702

I have two fixes for this, one reverting the changes from #3141 in #3266 and another one with disabling the obsolete tests in #3267 . Which one should we pick @caponetto @shalberd

@shalberd
Copy link
Contributor

shalberd commented Dec 31, 2024

@lresende @caponetto I'd go with the approach in PR 3267, that is: Fix CI without reverting removal of read_entry_data.
Happy New Year if we don't hear from each other in the next 24 hours :-) and thanks again y'all.

@lresende
Copy link
Member

lresende commented Jan 1, 2025

It looks like several tests depend on catalogs that are not working with this change, so I will have to revert it for now.

lresende added a commit that referenced this pull request Jan 1, 2025
…ector (#3141) (#3266)

* Revert "Remove obsolete read_entry_data method from class ComponentCatalogConnector (#3141)"

This reverts commit a60ae72.
This is breaking the build as it seems its not complete yet.

Signed-off-by: Luciano Resende <[email protected]>

* Update yarn.lock

Signed-off-by: Luciano Resende <[email protected]>

* Pin mistune

Signed-off-by: Luciano Resende <[email protected]>

---------

Signed-off-by: Luciano Resende <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:catalog connectors Access to component catalogs impact:breaking change Delivery introduces a change that is not backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove support for the read_catalog_entry method for component catalog connectors
5 participants