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.

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

Upstash shell plugin added #316

Merged
merged 6 commits into from Jul 5, 2023
Merged

Upstash shell plugin added #316

merged 6 commits into from Jul 5, 2023

Conversation

siddhikhapare
Copy link
Contributor

@siddhikhapare siddhikhapare commented Jun 25, 2023 •
edited

Overview

Type of change

  • Created a new plugin
  • Improved an existing plugin
  • Fixed a bug in an existing plugin
  • Improved contributor utilities or experience

Related Issue(s)

  • Resolves: #
  • Relates: #

How To Test

upstash auth login --email your-email --api-key your-apikey

test

Changelog

@AndyTitu AndyTitu added the waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member label Jun 26, 2023
hculea
hculea requested changes Jun 26, 2023
View reviewed changes
Copy link
Member

@hculea hculea left a comment

Choose a reason for hiding this comment

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

Great job 💪 Thank you for the contribution!

It looks like the civo and upstash plugins got both pushed in this PR, let's make sure to get rid of the former.

plugins/upstash/api_key.go Outdated Show resolved Hide resolved
plugins/upstash/upstash.go Show resolved Hide resolved
@techcraver
Copy link
Contributor

techcraver commented Jun 26, 2023 •
edited

Hello! Is this a submission for the 1Password Hackathon with Hashnode? If so, when you're ready, please be sure you write a blog post on Hashnode to make your submission official. Full instructions are on the Hackathon page.

AndyTitu
AndyTitu reviewed Jun 27, 2023
View reviewed changes
Copy link
Contributor

@AndyTitu AndyTitu left a comment

Choose a reason for hiding this comment

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

This looks mostly good to me! After all comments are resolved, this could be pretty close to the finish line

@AndyTitu AndyTitu added in-progress this PR is being worked on/comments are in the process of being addressed by the contributor and removed waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member labels Jun 27, 2023
hculea
hculea reviewed Jun 27, 2023
View reviewed changes
Copy link
Member

@hculea hculea left a comment

Choose a reason for hiding this comment

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

Looks great! Only one final comment.

plugins/upstash/api_key_test.go Show resolved Hide resolved
@hculea hculea added the hashnode hackathon Ideas and inspiration for the hackathon running from June 1st - June 30th label Jun 27, 2023
@siddhikhapare
Copy link
Contributor Author

@hculea @AndyTitu Done

@hculea
Copy link
Member

hculea commented Jun 27, 2023

Looks like there are still a few lint issues, otherwise looks good!

@siddhikhapare
Copy link
Contributor Author

@hculea Still I am getting error while doing make test. Something at TestAPIKeyImporter/config file but It's running fine and able to access with upstash.json

hculea
hculea reviewed Jun 29, 2023
View reviewed changes
plugins/civo/api_key.go Outdated Show resolved Hide resolved
hculea
hculea reviewed Jun 29, 2023
View reviewed changes
plugins/upstash/api_key.go Outdated Show resolved Hide resolved
plugins/upstash/api_key.go Outdated Show resolved Hide resolved
hculea
hculea approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@hculea hculea left a comment •
edited

Choose a reason for hiding this comment

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

The code looks good! 👍 Please gofmt your code to make the pipeline pass.

Please note that I only reviewed the code, and DID NOT functionally test. @arunsathiya @AndyTitu can I please ask you to do that? I currently have some troubles with my local setup.

@arunsathiya arunsathiya self-requested a review June 29, 2023 16:25
arunsathiya
arunsathiya reviewed Jun 29, 2023
View reviewed changes
Copy link
Contributor

@arunsathiya arunsathiya left a comment

Choose a reason for hiding this comment

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

I have taken this PR for a functional spin and it works well. Code-wise too, but there are some minor suggestions to address before it can be merged. Thanks for your contribution, @siddhikhapare!

plugins/upstash/api_key.go Show resolved Hide resolved
plugins/upstash/api_key_test.go Outdated Show resolved Hide resolved
plugins/upstash/upstash.go Outdated Show resolved Hide resolved
@arunsathiya
Copy link
Contributor

@siddhikhapare I am not seeing the changes in https://github.com/1Password/shell-plugins/pull/316/files 🤔

Could you share the commit ID? And a minor note: a generally good practice is to make the changes in a separate commit and not edit past commits and force-push. So, if you are doing the latter, please consider avoiding that going forward and take the former approach.

@siddhikhapare
Copy link
Contributor Author

@arunsathiya I have made changes

arunsathiya
arunsathiya approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@arunsathiya arunsathiya left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the changes @siddhikhapare!

@arunsathiya arunsathiya added waiting-on-sec-review and removed in-progress this PR is being worked on/comments are in the process of being addressed by the contributor labels Jun 29, 2023
@arunsathiya
Copy link
Contributor

@siddhikhapare Minor thing to fix: could you run gofmt -w . on the shell plugins directory, or more specifically gofmt -s -w plugins/upstash/*? That sound point you to an additional space that needs to be fixed.

If you are on VS code, I highly recommend the Go extension as it automatically takes care of formatting.

@siddhikhapare
Copy link
Contributor Author

@siddhikhapare Minor thing to fix: could you run gofmt -w . on the shell plugins directory, or more specifically gofmt -s -w plugins/upstash/*? That sound point you to an additional space that needs to be fixed.

If you are on VS code, I highly recommend the Go extension as it automatically takes care of formatting.

@arunsathiya Thank you for your suggestions.

@jpcoenen
Copy link
Member

jpcoenen commented Jul 4, 2023

Thanks for your contribution! Looks good to me 🚀

The commit I pushed is to add a missing newline; seemed silly to post a comment for that.

@hculea hculea merged commit 8df60af into 1Password:main Jul 5, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arunsathiya arunsathiya arunsathiya approved these changes

@AndyTitu AndyTitu AndyTitu left review comments

@jpcoenen jpcoenen jpcoenen approved these changes

@hculea hculea hculea approved these changes

Assignees

@siddhikhapare siddhikhapare

Labels
hashnode hackathon Ideas and inspiration for the hackathon running from June 1st - June 30th
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@siddhikhapare @techcraver @hculea @arunsathiya @jpcoenen @AndyTitu