Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

zapier convert: Add .environment file and instruction to edit it #240

Merged
merged 2 commits into from
Feb 20, 2018

Conversation

eliangcs
Copy link
Member

@eliangcs eliangcs commented Feb 14, 2018

This PR generates a .environment file incluing all the auth fields and prints a message instructing users to edit .environment when the conversion is done.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@eliangcs eliangcs requested a review from xavdid February 14, 2018 08:46
Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

Sweet! So the code looks good. My biggest feedback is the name of the file. The standard in node is to store this info in the .env instead of .environment. In fact, it doesn't look like the latter is standard in any of the libs (node or python) that might read this file.

So at the very least, I think we should change the filename to be .env. The next step would be to change the default behavior of zapier.tools.inject() to read .env, which is a breaking change. easy fix though, and since we're gearing up for a Major anyway, this is a good time for it.

Other than that, looks great!

@eliangcs
Copy link
Member Author

eliangcs commented Feb 19, 2018

@xavdid I agree that it's nicer to follow the mainstream convention and use .env, but I don't think it deserves a breaking change. We can make it backward compatible by keeping supporting .environment (but shows a warning message) when .env is absent, so we're not forced to include it in our next release. Does that make sense? Created PDE-76 to track this.

@xavdid
Copy link
Contributor

xavdid commented Feb 19, 2018

I'm not quite following, why would we show a warning? If a dev uses .environment, they would expect a silent run.

Anyway I think a major release is a perfect time to change this. We're using something non-standard, so updating the behavior with a better default makes sense. As long as we're clear in the notes, I wouldn't worry about it. If you're calling it without args, your tests will fail. Easy fix though (rename file or pass are).

Alternatively, we could check for both files? Or overlap for a major and then remove .environment on the next?

Either way, i think adhering to industry standards is a worthwhile breaking change

@eliangcs
Copy link
Member Author

eliangcs commented Feb 19, 2018

My intent is to avoid expanding the planned scope of our next release (5.0.0) because I think we already have enough todos in our task queue. So I'm suggesting NOT to include the .environment -> .env change in the next release (5.0.0). Rather, we make it backward compatible by checking both files: 1) attempt to load .env first and 2) load .environment if .env isn't found. Since in this way it's not a breaking change anymore, we can release it in any non-major versions we want, say 5.1.0.

@xavdid
Copy link
Contributor

xavdid commented Feb 19, 2018

Got it! Sounds good to me.

@eliangcs eliangcs merged commit ac2d65d into master Feb 20, 2018
@eliangcs eliangcs deleted the convert-env-instruction branch February 20, 2018 02:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants