1Password / op-js

A JS library powered by the 1Password CLI
https://developer.1password.com/docs/cli
MIT License
91 stars 8 forks source link

anujchoudhary17 - Section interface should have a label field #105 Fixed #133

Closed anujchoudhary-17 closed 1 year ago

anujchoudhary-17 commented 1 year ago

Added label string as an optional variable field in the Section interface. In the op-js/src/index.ts file I have made the following changes. Please review it and let me know if any further changes are required :) image

jodyheavener commented 1 year ago

Ah, apologies - @anujchoudhary-17 before I can merge your commits must be signed. If you are new to Git commit signing, check out the 1Password's SSH Agent!

anujchoudhary-17 commented 1 year ago

Ah, apologies - @anujchoudhary-17 before I can merge your commits must be signed. If you are new to Git commit signing, check out the 1Password's SSH Agent!

Sure will check it and will take care of this from now onwards 😊

anujchoudhary-17 commented 1 year ago

Hi @jodyheavener , I followed all the mentioned steps in the documentation. Please check whether everything is okay from my side. Thank you :)

anujchoudhary-17 commented 1 year ago

Sorry @jodyheavener , This took a while to figure out the Authentication Keys and Signing Keys on GitHub. Finally, I can see the Verified commit now 😀

jodyheavener commented 1 year ago

Thanks for this @anujchoudhary-17 - unfortunately all commits in the PR need to be signed in order to allow merging, and only your most recent one is signed. Because this change is quite simple the easiest solution here would be to squash them together and make sure the single resulting commit is signed.

anujchoudhary-17 commented 1 year ago

Hi @jodyheavener , I performed the squash action image Is this the desired output?

jodyheavener commented 1 year ago

Hi @jodyheavener , I performed the squash action image Is this the desired output?

That's the correct command to start an interactive rebase. This should open a terminal window to make changes to the last 3 commits, and you should squash the last 2 into the first one to create one final commit. You can see if it worked by calling git log afterwards to see your single commit.

anujchoudhary-17 commented 1 year ago

@jodyheavener
image Yes that is correct I can see only one commit from my side and I did squash 2 of my commits Is there anything else need to do from my side?

jodyheavener commented 1 year ago

@anujchoudhary-17 nice job! That looks correct from my end. Now you just need to push these changes. Because you "rewrote history" git won't let you push regularly, so you'll need to use --force (or preferably --force-with-lease).

anujchoudhary-17 commented 1 year ago

I hope I did the right way this time

jodyheavener commented 1 year ago

Hey @anujchoudhary-17! It looks like your older commits are still present. These will either need to be dropped/removed or squashed. You can do this via interactive rebase as well. If you run git log you can verify if the commits are still present.

anujchoudhary-17 commented 1 year ago

@jodyheavener Could we please have a meeting to solve these issues? Please let me know what time suits you :)

anujchoudhary-17 commented 1 year ago

@jodyheavener tried to do what you said is it correct now?

edif2008 commented 1 year ago

Unfortunately, it seems that when creating a PR from a forked branch, when you do a force push on the original branch, on the branch that has the PR it will perform a merge automatically. This means that your past commits were not overwritten.

What I do suggest we do on both sides is to close this PR and open a new one with the commit(s) that are now signed and that have the proper changes.

jodyheavener commented 1 year ago

Apologies for the delay, @anujchoudhary-17 - as @edif2008 suggested let's try opening a new PR with your new signed commits.