feat: search current working directory for config file by IndexSeek · Pull Request #1464 · apache/iceberg-python

IndexSeek · 2024-12-22T21:09:47Z

Resolves #1333

Adds the current working directory to the search path for the .pyiceberg.yaml file.

As it is now, the file is searched in the following order:

the PYICEBERG_HOME environment variable
~/
./

I'm unsure if people would like to have 2 and 3 swapped. In either case, users can still override this with the environment variable.

kevinjqliu

Thanks for the PR! Added a few nit comments

pyiceberg/utils/config.py

kevinjqliu · 2024-12-22T21:28:54Z

mkdocs/docs/configuration.md

there are a couple other places where pyiceberg.yaml is referenced in the docs
https://grep.app/search?q=pyiceberg.yaml&filter[repo][0]=apache/iceberg-python&filter[path][0]=mkdocs/docs/

Nice finds! That grep tool is pretty neat. I just made some corrections to these in 3ebbb8c.

kevinjqliu · 2024-12-22T21:29:41Z

pyiceberg/utils/config.py

nit: also add test https://grep.app/search?q=pyiceberg.yaml&filter[repo][0]=apache/iceberg-python&filter[path][0]=tests/

I tried adding a test here, but I wonder if there are opportunities to clean it up.

kevinjqliu

Thanks for the PR! I added some nit comments on testing

tests/utils/test_config.py

IndexSeek · 2024-12-26T15:10:39Z

There's another test that looks to do something similar to this newer test. Are both needed?

iceberg-python/tests/utils/test_config.py

Lines 58 to 65 in 0e5086c

    
           def test_from_configuration_files(tmp_path_factory: pytest.TempPathFactory) -> None: 
        
               config_path = str(tmp_path_factory.mktemp("config")) 
        
               with open(f"{config_path}/.pyiceberg.yaml", "w", encoding=UTF8) as file: 
        
                   yaml_str = as_document({"catalog": {"production": {"uri": "https://service.io/api"}}}).as_yaml() 
        
                   file.write(yaml_str) 
        
               os.environ["PYICEBERG_HOME"] = config_path 
        
               assert Config().get_catalog_config("production") == {"uri": "https://service.io/api"}

mkdocs/docs/cli.md

Fokko

I've left one comment, but apart from that, it looks good to me 👍

IndexSeek · 2024-12-29T18:00:17Z

I've left one comment, but apart from that, it looks good to me 👍

Thank you! I applied that change! ✅

kevinjqliu

LGTM! I have a few nit comments about testings and adding a warning for storing in current dir

kevinjqliu · 2025-01-02T20:41:43Z

mkdocs/docs/configuration.md

 There are three ways to pass in configuration:

- Using the `~/.pyiceberg.yaml` configuration file
+- Using the `.pyiceberg.yaml` configuration file stored in either the directory specified by the `PYICEBERG_HOME` environment variable, the home directory, or current working directory.


nit: move the extra info about where the file is located down to L37.
also i think its valuable to include a warning about accidentally checking in secrets with git when using the current working directory

kevinjqliu · 2025-01-02T20:42:30Z

mkdocs/docs/api.md

 and loaded in python by calling `load_catalog(name="hive")` and `load_catalog(name="rest")`.

-This information must be placed inside a file called `.pyiceberg.yaml` located either in the `$HOME` or `%USERPROFILE%` directory (depending on whether the operating system is Unix-based or Windows-based, respectively) or in the `$PYICEBERG_HOME` directory (if the corresponding environment variable is set).
+This information must be placed inside a file called `.pyiceberg.yaml` located either in the `$HOME` or `%USERPROFILE%` directory (depending on whether the operating system is Unix-based or Windows-based, respectively), in the current working directory, or in the `$PYICEBERG_HOME` directory (if the corresponding environment variable is set).


nit: include warning about accidentally checking in secrets with git when using the current working directory

tests/utils/test_config.py

kevinjqliu · 2025-02-01T00:07:33Z

hey @IndexSeek could you rebase this PR and run make lint? would be great to include this as part of the 0.9.0 release

IndexSeek · 2025-02-01T16:59:05Z

hey @IndexSeek could you rebase this PR and run make lint? would be great to include this as part of the 0.9.0 release

I just rebased and pre-commit is passing. Please let me know if you want me to make additional changes. I'm looking forward to getting this one in!

Fokko · 2025-02-03T21:07:43Z

@IndexSeek It looks like the CI is still a bit sad, do you have a minute to look at this?

kevinjqliu · 2025-02-03T21:46:02Z

its an issue with dead url link, i think rebasing main should resolve it.

Also, do you think its important to include a warning about accidentally checking in secrets with git when using the current working directory?

Co-authored-by: Fokko Driesprong <fokko@apache.org>

kevinjqliu · 2025-02-12T04:47:10Z

rebased off main and fixed the tests.

Thanks @IndexSeek for the contribution and @Fokko for the review :)

kevinjqliu reviewed Dec 22, 2024

View reviewed changes

kevinjqliu reviewed Dec 23, 2024

View reviewed changes

tests/utils/test_config.py Outdated Show resolved Hide resolved

tests/utils/test_config.py Outdated Show resolved Hide resolved

tests/utils/test_config.py Outdated Show resolved Hide resolved

Fokko reviewed Dec 29, 2024

View reviewed changes

mkdocs/docs/cli.md Outdated Show resolved Hide resolved

Fokko approved these changes Dec 29, 2024

View reviewed changes

kevinjqliu reviewed Jan 2, 2025

View reviewed changes

kevinjqliu added this to the PyIceberg 0.9.0 release milestone Jan 8, 2025

IndexSeek force-pushed the add-cwd-config-route branch from 6d14ece to 51ab8e2 Compare February 1, 2025 16:56

IndexSeek and others added 7 commits February 11, 2025 19:53

feat: search current working directory for config file

90802c5

test: add config file search path for cwd

d107647

docs: add docs on new .pyiceberg.yaml search path

e40accd

tests: fix up mypy issues on tests

21677ff

tests: ensure multiple lookup resolution is valid and UTF8 usage

36d8409

Update mkdocs/docs/cli.md

dfacdcd

Co-authored-by: Fokko Driesprong <fokko@apache.org>

reword docs and make lint

e3d1a63

kevinjqliu force-pushed the add-cwd-config-route branch from 51ab8e2 to e3d1a63 Compare February 12, 2025 04:00

kevinjqliu added 2 commits February 11, 2025 20:22

rewrite failed tests

539b1d1

ignore mypy recursivedict error

7e0b29f

kevinjqliu merged commit 86b83e8 into apache:main Feb 12, 2025
8 checks passed

Conversation

IndexSeek commented Dec 22, 2024

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

IndexSeek Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

IndexSeek Dec 22, 2024

Choose a reason for hiding this comment

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IndexSeek commented Dec 26, 2024

Uh oh!

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

IndexSeek commented Dec 29, 2024

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Jan 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu commented Feb 1, 2025

Uh oh!

IndexSeek commented Feb 1, 2025

Uh oh!

Fokko commented Feb 3, 2025

Uh oh!

kevinjqliu commented Feb 3, 2025

Uh oh!

Uh oh!

kevinjqliu commented Feb 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants