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

feat(auth): replace auth mechanism: migrate away from oob & use modern libraries #201

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wescpy
Copy link
Contributor

@wescpy wescpy commented Mar 21, 2023

  • Replace use of google-auth-library with @google-cloud/local-auth
  • Remove use of oob OAuth2 flow which was deprecated Feb 2022
  • Remove unused pkgs per npx depcheck
  • Updated tests so they pass
  • Remaining work: use of regular fs and fs.promises should be made consistent (pick one)

@google-cla
Copy link

google-cla bot commented Mar 21, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@wescpy
Copy link
Contributor Author

wescpy commented Mar 21, 2023

This patch should fix </issues/170> and </issues/199>. Besides the obvious fix of replacing urn:ietf:wg:oauth:2.0:oob with http://localhost (pushed under the covers so you don't even see this), it also replaces the entire auth chunks of the current solution with the more nimble version from the Quickstart samples in the documentation.

@py9mrg
Copy link

py9mrg commented Aug 21, 2023

Hello @wescpy

I see you’re still working wonders trying to keep this useful tool functional! I was wondering if you’d consider forking it completely and maintaining your own version with all the info up to date etc etc? Given how glacial progress seems to be to incorporate your PR, and with the “documentation” of how to get this to work being spread across the main readme, some issue comments, and this PR, it’s pretty confusing what should be the up to date approach for installation.

I think this would be far better served as a forked repos that completely supersedes this one, with an up to date readme, release (or at least change the readme to reflect installation from GitHub rather than npm if you don’t want to go down that route). Albeit I appreciate that’s putting a lot of work and responsibility on you, but otherwise this tool is basically dead for anyone who doesn’t want to ease through the disparate installation instructions.

@wescpy
Copy link
Contributor Author

wescpy commented Aug 22, 2023

Howdy, and thanks for your interest in this project. As this is a PR, I have a fork already which you or anyone else can use to get a working version from me while I await for this PR to be reviewed and accepted.

As this is repo & project were originally meant to be a larger demo of the Google Slides REST API, it's currently being maintained by the Google employee who created the code sample. He's quite busy and only able to maintain this repo on a "best-effort" basis. As he hasn't indicated this project has been abandoned, I'm happy to continue making changes in my fork to keep a working version going for a few reasons:

  • Plain text with markup to slide deck is a passion of mine, so happy to work on this
  • I've also been learning Node.js/Typescript the past few years, and working on this has helped develop my skills with that tooling as my primary expertise is Python
  • Twenty-five years ago, I created a similar tool in Python for Powerpoint (albeit Markdown didn't exist back then, so I just made up some light wiki-style markup) as part of my mainline Python book series http://corepython.com ... I'll be putting all that code on GH soon as I update it, and hopefully one day to create a full Markdown version of that Python/Powerpoint script

That said, while I'll likely be able to update my fork more regularly than the official repo, I'm only able to provide a "best effort" response too, unless others want to help maintain and improve it. Happy to continue discussing.

@py9mrg
Copy link

py9mrg commented Aug 24, 2023

Great, thanks. Yes I saw your fork but - correct me if I'm wrong - the installation instructions are for the original version as they indicate using npm? So perhaps you could update those otherwise new users are still having to hunt through the issues etc to find the correct method?

For me it's particularly tricky as my work doesn't allow us to make a Google project, so I have to make a personal one and then share that. But as someone very new to Google projects etc, it took me quite a while to realise that what with all the other things needed to get it working. Speaking of which, does the new Auth system still require a Google project to get your credentials?

As someone coming for the R ecosystem where you have Rmarkdown (which allows basic Office support) and then the excellent officer suite of packages to convert to improve support for converting to word, PowerPoint etc - I can definitely agree on how useful that is. Speaking of which, the Rmarkdown project has largely been superseded by Quarto, which also supports Python (and Julia). I don't know how well in terms of exporting to PowerPoint but you might take a look as your tools might integrate with that well in a similar way officer does for the R side.

@wescpy
Copy link
Contributor Author

wescpy commented Aug 25, 2023

TL;DR: If you want a working version of this tool without waiting for this PR to be merged, see https://github.com/wescpy/md2googleslides or run npm install -g @wescpy/md2gslides.

LONG:
Ah, I see what you're saying now. Ok, I just updated the repo that brings it closer to what you're looking for:

  1. Updated all the NPM packages to the latest available (see package-lock.json and yarn.lock)
  2. Changed to scoped package with my login (@wescpy/md2gslides) to preserve original name and bumped version from 0.5 (original) and 0.5.1 (my original fix) to 0.6.0 (this new release); see package.json.
  3. Published to NPM with updated README installation instructions

I just ran an npm install with my published package and generated a slide deck successfully, so please give it a try. There are many deprecation warnings during the build, so if you want to fix those, be my guest!

One issue I need help with: Now that I've "polluted" my fork with a different package name and version number, this PR shouldn't be accepted as-is and merged upstream. Is there a way to flag that the most critical files like README.md, package.json, and yarn.lock shouldn't be part of the PR?

To answer one of your questions: you must create a Google project in order to use their APIs and get usable credentials. I'll take a look at Quarto/Python when I get a chance. I'm updating my books (see http://corepython.com) where the PowerPoint code lives... also need to get all those samples into GitHub too.

@wescpy wescpy changed the title migrate away from oob, replace auth & remove unused pkgs feat(auth): replace auth mechanism: migrate away from oob, use modern libraries, remove unused pkgs Aug 26, 2023
@wescpy wescpy changed the title feat(auth): replace auth mechanism: migrate away from oob, use modern libraries, remove unused pkgs feat(auth): replace auth mechanism: migrate away from oob & use modern libraries Aug 26, 2023
@py9mrg
Copy link

py9mrg commented Aug 26, 2023

Thank you so much for that! If I can get this completely working it's going to be such a time saver because my company basically communicates everything with G slides these days.

I'm going to completely remove and start again to see how it goes when I get a chance (away travelling at the moment, do have laptop but it's not so easy to find the time). One other thing I've just remember is this issue. Sorry to bring it up but it's been so long since I tried md2gslides I forgot I was having this.

I didn't realise at the time (not sure how I missed it) but someone already posted a similar issue that has been marked bug. I'm on my phone right now so will edit this comment with that issue as well. I realise I'm being very cheeky now though so don't feel pressured to address it as the person has a solution for it (I know zero js so I couldn't grasp it).

Edit: here it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants