Skip to content

fix(__getattr__): non-existent attribute lookup #982

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 18, 2023
Merged

fix(__getattr__): non-existent attribute lookup #982

merged 1 commit into from
Jun 18, 2023

Conversation

m4dh4t
Copy link
Contributor

@m4dh4t m4dh4t commented Apr 25, 2023 •
edited by briantist
Loading

Fix #838
Closes #839

@m4dh4t m4dh4t requested a review from a team as a code owner April 25, 2023 12:57
@m4dh4t m4dh4t mentioned this pull request Apr 25, 2023
Closed
@briantist briantist self-assigned this May 4, 2023
@briantist briantist added secrets engines generally related to a Vault secrets engine kv Key/Value (KV) secrets engine bug labels May 4, 2023
@codecov
Copy link

codecov bot commented May 4, 2023 •
edited
Loading

Codecov Report

Merging #982 (75ab2c6) into main (31aca14) will increase coverage by 0.05%.
The diff coverage is 66.66%.

❗ Current head 75ab2c6 differs from pull request most recent head 5e5aa46. Consider uploading reports for the commit 5e5aa46 to get more accurate results

@@            Coverage Diff             @@
##             main     #982      +/-   ##
==========================================
+ Coverage   81.98%   82.03%   +0.05%     
==========================================
  Files          65       65              
  Lines        3019     3023       +4     
==========================================
+ Hits         2475     2480       +5     
+ Misses        544      543       -1
Impacted Files Coverage Δ
hvac/api/secrets_engines/kv.py 97.05% <50.00%> (-2.95%) ⬇️
hvac/api/vault_api_category.py 93.02% <50.00%> (-2.10%) ⬇️
hvac/api/auth_methods/token.py 83.33% <100.00%> (ø)

... and 1 file with indirect coverage changes

briantist
briantist reviewed May 4, 2023
View reviewed changes
Copy link
Contributor

@briantist briantist left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this!

Please include test coverage for the new lines as well.

@briantist briantist force-pushed the develop branch 3 times, most recently from 085b858 to 9dbfa98 Compare May 10, 2023 06:41
@briantist
Copy link
Contributor

Hello! The develop branch has recently had some changes, and your branch will need to be rebased and pushed again. Thanks and sorry for the inconvenience.

@m4dh4t
Copy link
Contributor Author

m4dh4t commented May 17, 2023

Sorry for the delay, I just updated the condition in kv.py. Here is the latest coverage report:

---------- coverage: platform linux, python 3.10.6-final-0 -----------
Name                                              Stmts   Miss  Cover
---------------------------------------------------------------------
hvac/__init__.py                                      2      0   100%
hvac/adapters.py                                    105     11    90%
hvac/api/__init__.py                                  6      0   100%
hvac/api/auth_methods/__init__.py                    26      3    88%
hvac/api/auth_methods/approle.py                     87      0   100%
hvac/api/auth_methods/aws.py                        157     67    57%
hvac/api/auth_methods/azure.py                       47      0   100%
hvac/api/auth_methods/cert.py                        67      6    91%
hvac/api/auth_methods/gcp.py                         78      8    90%
hvac/api/auth_methods/github.py                      41      0   100%
hvac/api/auth_methods/jwt.py                         39      3    92%
hvac/api/auth_methods/kubernetes.py                  47      1    98%
hvac/api/auth_methods/ldap.py                        57      7    88%
hvac/api/auth_methods/legacy_mfa.py                  31     20    35%
hvac/api/auth_methods/mfa.py                         37      3    92%
hvac/api/auth_methods/oidc.py                         6      0   100%
hvac/api/auth_methods/okta.py                        47      0   100%
hvac/api/auth_methods/radius.py                      40     28    30%
hvac/api/auth_methods/token.py                       72     12    83%
hvac/api/auth_methods/userpass.py                    27      9    67%
hvac/api/secrets_engines/__init__.py                 20      0   100%
hvac/api/secrets_engines/active_directory.py         30     18    40%
hvac/api/secrets_engines/aws.py                      52      2    96%
hvac/api/secrets_engines/azure.py                    35      0   100%
hvac/api/secrets_engines/consul.py                   26     15    42%
hvac/api/secrets_engines/database.py                 56     36    36%
hvac/api/secrets_engines/gcp.py                      60     31    48%
hvac/api/secrets_engines/identity.py                267     16    94%
hvac/api/secrets_engines/kv.py                       34      1    97%
hvac/api/secrets_engines/kv_v1.py                    29      0   100%
hvac/api/secrets_engines/kv_v2.py                    99      0   100%
hvac/api/secrets_engines/pki.py                     133      0   100%
hvac/api/secrets_engines/rabbitmq.py                 26     15    42%
hvac/api/secrets_engines/ssh.py                      61      4    93%
hvac/api/secrets_engines/transform.py               128     90    30%
hvac/api/secrets_engines/transit.py                 141     13    91%
hvac/api/system_backend/__init__.py                  22      0   100%
hvac/api/system_backend/audit.py                     20      1    95%
hvac/api/system_backend/auth.py                      38      5    87%
hvac/api/system_backend/capabilities.py              14     11    21%
hvac/api/system_backend/health.py                    14      2    86%
hvac/api/system_backend/init.py                      32     14    56%
hvac/api/system_backend/key.py                       87     11    87%
hvac/api/system_backend/leader.py                     8      2    75%
hvac/api/system_backend/lease.py                     26      3    88%
hvac/api/system_backend/mount.py                     38      6    84%
hvac/api/system_backend/namespace.py                 12      6    50%
hvac/api/system_backend/policy.py                    21      0   100%
hvac/api/system_backend/raft.py                      24     14    42%
hvac/api/system_backend/seal.py                      26      0   100%
hvac/api/system_backend/system_backend_mixin.py       5      0   100%
hvac/api/system_backend/wrapping.py                   8      0   100%
hvac/api/vault_api_base.py                            6      0   100%
hvac/api/vault_api_category.py                       43      3    93%
hvac/aws_utils.py                                    37      2    95%
hvac/constants/__init__.py                            0      0   100%
hvac/constants/approle.py                             3      0   100%
hvac/constants/aws.py                                 6      0   100%
hvac/constants/azure.py                               2      0   100%
hvac/constants/client.py                              8      0   100%
hvac/constants/gcp.py                                 7      0   100%
hvac/constants/identity.py                            2      0   100%
hvac/constants/transit.py                            11      0   100%
hvac/exceptions.py                                   38      0   100%
hvac/utils.py                                        92     15    84%
hvac/v1/__init__.py                                 157     18    89%
---------------------------------------------------------------------
TOTAL                                              3023    532    82%

@briantist
Copy link
Contributor

Hi @m4dh4t , unfortunately the wrong commits from the branch issues are still included; a merge from develop into the branch will not remove them, so you may have to do an interactive rebase. See the commit list to see all the commits that shouldn't be there.

With the interactive rebase, you could remove the commits that shouldn't be there and only include the commits that are part of your PR.

You could try a workflow like this to achieve that:

# save the current state of the branch in a new branch
git checkout -b save-branch
# switch back to your original
git checkout -
# interactive rebase
git rebase -i origin/develop

If you're having trouble, I can try to pull down your branch and fix it when I get some time.

@m4dh4t m4dh4t force-pushed the fix-838 branch 3 times, most recently from a809990 to fc9277d Compare May 30, 2023 07:33
@m4dh4t
Copy link
Contributor Author

m4dh4t commented May 30, 2023

Hi @briantist, thanks for pointing this out I rebased the branch accordingly. Let me know if there is anything else needed.

@briantist briantist changed the base branch from develop to main June 17, 2023 20:50
@m4dh4t @briantist
fix(__get_attr__): non-existant attribute lookup
5e5aa46 
fix(__get_attr__): typo

Explicit default_kv_version AttributeError
@briantist
Copy link
Contributor

Hi @m4dh4t , I'm sorry to report another major change in the project's git/branch/developer workflow, but this is hopefully the last one.

The hvac project is simplifying its developer workflow and removing the develop branch. Please be aware for future PRs that the project's default branch is now main.

Since your PR was opened before this change, I have:

  • updated your PR to merge into the project's main branch
  • rebased your branch against main and force-pushed it (please pull before making any further changes)

I hope to review this PR soon as I take care of some of the project/repository pain points, which should free up more time for reviews. Thank you for your patience!

@briantist briantist mentioned this pull request Jun 17, 2023
Closed
@briantist briantist changed the title fix(__get_attr__): non-existant attribute lookup fix(__get_attr__): non-existent attribute lookup Jun 18, 2023
@briantist briantist changed the title fix(__get_attr__): non-existent attribute lookup fix(__getattr__): non-existent attribute lookup Jun 18, 2023
briantist
briantist approved these changes Jun 18, 2023
View reviewed changes
Copy link
Contributor

@briantist briantist left a comment

Choose a reason for hiding this comment

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

I finally got a chance to dig into this, really nice analysis in the issue, thanks for the fix and the link to the coverage instance of the same problem.

@briantist briantist merged commit 3b4e5ca into hvac:main Jun 18, 2023
@m4dh4t
Copy link
Contributor Author

m4dh4t commented Jun 18, 2023

Thanks for the quick review after the last project changes, glad to have contributed to the project !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briantist briantist briantist approved these changes

Assignees

@briantist briantist

Labels
bug kv Key/Value (KV) secrets engine secrets engines generally related to a Vault secrets engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Recursion error in VaultApiCategory.__getattr__()
2 participants
@m4dh4t @briantist