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

Creating logic to update lock codes #222

Closed
wants to merge 6 commits into from

Conversation

shwarnock
Copy link

No description provided.

@shwarnock
Copy link
Author

pre-commit.ci autofix

@shwarnock
Copy link
Author

I am not sure why It says Merging blocked. LMK what I need to do to fix this

Copy link
Owner

@tykeal tykeal left a comment

Choose a reason for hiding this comment

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

This logic needs to change.

  1. The update should only happen if the code generator is the date_based generator
  2. You shouldn't be directly manipulating the code, but instead causing the sensor to fire and async_fire_clear_code on the specific slot. That will cause the slot to be cleared in Keymaster and then on the next refresh of the sensor it will be refilled with an updated door code.

@tykeal
Copy link
Owner

tykeal commented Nov 7, 2024

@shwarnock the merge block is because you did not sign your changes. See https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

If you don't have a GPG key to use, then see near the bottom the information about SSH based signing.

Please note, your current changes are going to run afoul of pre-commit expectations around commit messages

@tykeal
Copy link
Owner

tykeal commented Nov 7, 2024

BTW if you're wondering why I've got such strict requirements on the repo it's because my day job is working with OSS developers. In particular, around coding best practices as well as CI tooling so I maintain the same level of expectations of my own coding projects ;)

@shwarnock
Copy link
Author

@shwarnock the merge block is because you did not sign your changes. See https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

If you don't have a GPG key to use, then see near the bottom the information about SSH based signing.

Please note, your current changes are going to run afoul of pre-commit expectations around commit messages

I did add an SSH key to my account. Does it specifically need to be GPG?

@shwarnock
Copy link
Author

@shwarnock the merge block is because you did not sign your changes. See https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

If you don't have a GPG key to use, then see near the bottom the information about SSH based signing.

Please note, your current changes are going to run afoul of pre-commit expectations around commit messages

Sorry, it has been a long time since I have used git. I have been in P4 for the past 10 years and only rarely come back to Git. I added a GPG key to my account and used git commit -S -m "Comment" to commit my changes, but my commits are still saying unverified.

Really excited to get this all sorted out! I would love to help out with this integration more. I already have 2 STRs that are using this integration with more to come in the coming months!

@shwarnock
Copy link
Author

pre-commit.ci autofix

tykeal and others added 6 commits November 7, 2024 15:38
Starting with Home Assistant 2024.7 the async_forward_entry_setups
method must be awaited or we get a deprecation notice.

Issue: Fixes tykeal#211
Signed-off-by: Andrew Grimberg <tykeal@bardicgrove.org>
Signed-off-by: shwarnock <shwarnock89@gmail.com>
Signed-off-by: Shawn Warnock <shwarnock89@gmail.com>
Signed-off-by: shwarnock <shwarnock89@gmail.com>
Signed-off-by: shwarnock <shwarnock89@gmail.com>
Signed-off-by: shwarnock <shwarnock89@gmail.com>
Signed-off-by: shwarnock <shwarnock89@gmail.com>
Signed-off-by: shwarnock <shwarnock89@gmail.com>
@tykeal
Copy link
Owner

tykeal commented Nov 8, 2024

@shwarnock ok... it looks like you're fumbling around a little bit here given what I'm seeing on all the commits (don't worry, I understand, especially if you don't use git on a regular basis!)

First, let's take care of your signing issue:

https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification#ssh-commit-signature-verification

that's the section specifically about SSH based signing of commits. Make sure you upload the public key for your SSH key as a signing key per that documentation and then also make sure you configured user.singingKey correctly per https://git-scm.com/docs/git-config#Documentation/git-config.txt-usersigningKey

As an aside, I just reread that you used a GPG key, great, but you do need to upload the public side of the key to your GitHub account so GitHub knows that the key is actually yours.

Now, on to all of your commits. Looks like you somehow grabbed the last commit I made and added a DCO (signed-off-by) line to it, that's going to break all sorts of things since that commit is already merged!

So, here's what I would do, the following assumes that you have this repo (mine) configured as an upstream remote which it would be if you cloned / forked using the gh CLI utility.

# cleanup and make sure main is still in sync with upstream
git checkout main && git fetch upstream
git reset --hard 5c5f7cf64b2b7238565f227ef99615a68c2c80c9
# move the branch I was working on out of the way
git branch -m slot-codes-update slot-codes-update-old
# get a new fresh feature branch
git checkout -b slot-codes-update

General house keeping tip for you. This repository uses pre-commit to make sure that commits are following good practices, if you have not done so, then I would suggest installing pre-commit and in the repo doing the following:

pre-commit install
pre-commit install -t commit-msg

Doing this once before you start making changes will make sure that all of the pre-commit related CI jobs will pass as you won't be able to get your change committed locally until you pass them ;)

With that all out of the way, let's talk about the actual code you're trying to change.

  1. Your latest changes call async_fire_clear_code with no parameters. This is going to fail as you have to tell it which slot code to clear.

  2. We currently do not have a handler to get the slot number that is currently assigned to a given name, just to get the data that a slot with a given name has, so... this has become a bit more complex than I originally though :-/ we'll need a commit that adds a get_slot_number_by_name operator to the event_overrides.py

  3. In the calsensor, you'll want to then capture the slot number and then after the if update_times you'll want to fire off the await async_fire_clear_code call with the captured slot number. This should be a commit after the previous one as I subscribe to commits should be atomic in nature.

  4. After you've got all your changes made locally you can repush all of them back to this PR with:

git push origin slot-codes-update --force

Which will rewrite your current PR with all of the changes you've made and should also get around the issue I saw where you modified an already merge change!

@shwarnock
Copy link
Author

Thanks for the detailed reply! I actually just created a new PR and branch that is less poluted: https://github.com/tykeal/homeassistant-rental-control/pull/223. I believe I got the signing working properly. I did notice that if you allow the pre-commit tool auto-fix an issue (supplying the comment to the PR) the commit that tool submits is unverified, so it causes Merging Blocked message to show up. I had to revert the tool's commit and submit its changes myself. This was difficult to do without first submitting and then reverting, because the details link for the pre-commit.ci checks doesn't actually tell you what it is changing.

@shwarnock shwarnock closed this Nov 9, 2024
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.

2 participants