bring your own credentials (issue #64) #74
Conversation
There was a problem hiding this comment.
Super amazing work, works smoothly ❤️
I propose couple of improvements though, let me know what you think. Hopefully, this will help us to have this merged!
I extended your code a little bit here: https://github.com/gsuitedevs/md2googleslides/compare/master...bwplotka:auth?expand=1 and commented here all the feedback.
Hello maintainers! 👋
Is this project maintained? Do you need any help in maintaining this? Is there any communication channel we can discuss where would you need help particularly, what's the vision?
It looks like this is amazing base for further growth and flexibility in defining slides in markdown code... we would love to jump in and start helping more. Looks like people like @wescpy is happy to help as well 🤗
| var data; // needs to be scoped outside of try-catch | ||
| try { | ||
| data = fs.readFileSync(STORED_CLIENT_ID_PATH); | ||
| } catch(err) { | ||
| return console.log('Error loading client secret file:', err); | ||
| } | ||
| if(data === undefined) return console.log('Error loading client secret data:', err); | ||
| const creds = JSON.parse(data).installed; |
There was a problem hiding this comment.
Something that might help maintainers to get this merged is focusing on details, let's have this code more resilient. I think we have couple of things we could improve: hugs
- JSON parsing can error out easily, we should catch those! (:
data == undefinedis not very helpful. Also at this point we don't haveerrvariable anymore.
Also I am not the super experienced with typescript (Go dev here), is there more idiomatic way to check errors than exceptions? 🤔
What about something like this?
| var data; // needs to be scoped outside of try-catch | |
| try { | |
| data = fs.readFileSync(STORED_CLIENT_ID_PATH); | |
| } catch(err) { | |
| return console.log('Error loading client secret file:', err); | |
| } | |
| if(data === undefined) return console.log('Error loading client secret data:', err); | |
| const creds = JSON.parse(data).installed; | |
| var creds; | |
| if (!fs.existsSync(STORED_CLIENT_ID_PATH)) { | |
| return console.log('Client ID + Secret does not exists in path:', STORED_CLIENT_ID_PATH); | |
| } | |
| var data; | |
| try { | |
| data = fs.readFileSync(STORED_CLIENT_ID_PATH); | |
| creds = JSON.parse(data.toString()); | |
| } catch (err) { | |
| return console.log('Error loading client secret file:', err); | |
| } |
Tried here https://github.com/gsuitedevs/md2googleslides/compare/master...bwplotka:auth?expand=1 and works smoothly.
There was a problem hiding this comment.
Great! Looks good. The only thing I would change would be to shorten your warning string so line 134 is: return console.log('Credentials file missing from:', STORED_CLIENT_ID_PATH);, and a similar tweak can be made to line 142.
Interesting... your patch is suggesting having a backup of their credentials file (download via long hash name then make a copy w/the short name). Users can also choose to not keep the backup and simply rename the file. Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
|
BTW, I'm getting a CI/CD error... when Travis runs, it says, |
|
FWIW I tried these steps (I think I did them correctly) but it still prompts for authorization which is rejected. |
|
I suggest you try it again as at least 2 people (the committers) have used it successfully (and continue to do so... I used it to generate a presentation that was delivered a few weeks ago. Doublecheck you've applied the code patches to your install as it's not just the updated steps for using your own credentials. Hopefully the maintainer will have a free moment to review & accept this PR soon. |
One possible patch for </issues/64> where users are directed to the devconsole/API mgr and download their client ID & secret file as client_id.json. Store it in the same folder as their OAuth token files in ~/. md2googleslides.