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

Added support for environments on secrets command #95

Conversation

mitchelvanbever
Copy link

@mitchelvanbever mitchelvanbever commented Dec 11, 2021

First things first, awesome stuff going on here 🚀

Super happy to be able to contribute 🙂

I ran into something while debugging my wrangler-github-action-deployment secrets locally with wrangler V2.
It seemed like the environment flag wasn't being passed along, so I looked at the code and saw the todo comments so I thought I'd help out 😅

I've reversed engineered the endpoints by checking the dashboard while performing the same actions. I did check wrangler V1 code but couldn't make much sense out of that.

From what I've seen there are two different ways of retrieving secrets. One including variables (so non-secrets), the other one solely secrets. I've gone with the secrets only as it made more sense to me semantically but not sure which one of these endpoints you guys would prefer.

I've tested the GET / PUT / DELETE endpoints defined in this PR through insomnia and confirmed these routes to be listing the actual environment secrets.

I'll be adding tests as well just bear with me while I figure those out. I just wanted to get this out there for when something already required changes

TODO:

  • Add tests

@changeset-bot
Copy link

changeset-bot bot commented Dec 11, 2021

🦋 Changeset detected

Latest commit: 511c410

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mitchelvanbever
Copy link
Author

Having some trouble fixing the tests as they require user input (and a confirmation prompt). Any advice on how to best approach this? Ink doesn't like when I'm feeding inputs through stdin (something in regards to rawData).

Copy link
Contributor

@threepointone threepointone 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 the PR! Good job on reverse engineering from the dashboard haha. I can't merge this in just yet because we also need to support wrangler v1 style environments. I'll leave this open, marked as changes requested until I figure out details for that too. I'll also figure out something for tests. Thanks very much again!

@mitchelvanbever
Copy link
Author

Thanks for the PR! Good job on reverse engineering from the dashboard haha. I can't merge this in just yet because we also need to support wrangler v1 style environments. I'll leave this open, marked as changes requested until I figure out details for that too. I'll also figure out something for tests. Thanks very much again!

Thanks for checking it out!

Reason I created this PR is because it's not working on V1 or on V2 (previewing or publishing to different environments that rely on different secrets). So not sure if that's something that you're going to be able to support. But happy to update once you figured those details out 🙂

In regards to the tests, I think we would either need to set Ink to accept rawData (of which I don't know it's implications) or we could check-out the testing-library they've build specifically for testing ink apps. But I'll keep an eye on this MR.

Thanks again for taking the time to check this draft MR 😃

@huv1k
Copy link

huv1k commented Jan 11, 2022

Any new regarding this one? It's only blocker for us to go with wrangler2 😅

@threepointone
Copy link
Contributor

threepointone commented Feb 24, 2022

sorry for the delay on this. I have a comprehensive PR up at #523. Thank you for the initiative! Closing this for now, but I will credit you in the changelog!

@mitchelvanbever
Copy link
Author

sorry for the delay on this. I have a comprehensive PR up at #523. Thank you for the initiative! Closing this for now, but I will credit you in the changelog!

All good, your PR looks to very extensive me like it's including a lot more (including tests 😅).

Very kind to still give me some credits, but not sure if I've done all that much 😁

I'll keep an close eye on the repo anyways and will see if there is something I can help out with to make a sizeable contribution 👍

@github-actions github-actions bot mentioned this pull request Feb 24, 2022
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.

3 participants