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

docs: installation of the new iceberg_catalog_rest added to the docs #355

Merged

Conversation

nishant-sachdeva
Copy link
Contributor

No description provided.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks! We now have support for multiple catalogs. I'm guessing we need to reorganize the entire installation documentation. However, this PR itself is good to go.

@Xuanwo
Copy link
Member

Xuanwo commented Apr 27, 2024

Also cc @Fokko and @liurenjie1024 for a review.

@@ -23,10 +23,11 @@
Cargo 1.75.0 or later is required to build.
</div>

Add `iceberg` into `Cargo.toml` dependencies:
Add `iceberg` and `iceberg-catalog-rest` into `Cargo.toml` dependencies:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about splitting them into two sections as following:

Add `iceberg-catalog-rest` if you want to use rest catalog.

...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.
I'll push these changes in a day or two

@liurenjie1024
Copy link
Collaborator

cc @nishant-sachdeva Are you still working on this?

@Xuanwo
Copy link
Member

Xuanwo commented Jul 25, 2024

Hi, @liurenjie1024, this PR has been open for a while without any progress. I believe it's ready to be merged and we can continue to make improvements on it afterward. What do you think about merging it as is?

@liurenjie1024
Copy link
Collaborator

Hi, @liurenjie1024, this PR has been open for a while without any progress. I believe it's ready to be merged and we can continue to make improvements on it afterward. What do you think about merging it as is?

Seems @nishant-sachdeva is not responding. To be honest I still prefer to use different section for adding different dependencies. Or could we file other prs to add docs and close this pr first if @nishant-sachdeva doesn't have time to do the udpate?

@Xuanwo
Copy link
Member

Xuanwo commented Jul 26, 2024

Seems @nishant-sachdeva is not responding. To be honest I still prefer to use different section for adding different dependencies.

Understood.

Or could we file other prs to add docs and close this pr first if @nishant-sachdeva doesn't have time to do the udpate?

@nishant-sachdeva has made some effort in this PR and I do want to include his contribution in. I will finish this work based on his work on this PR.

@Xuanwo Xuanwo merged commit 35dac58 into apache:main Oct 12, 2024
6 checks passed
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