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

Use Discord Credentials when available #326

Merged
merged 10 commits into from
Jan 5, 2023

Conversation

KarthikRaju391
Copy link
Contributor

This is related to issue #105

  • Added Development setup for discord section in the documentation
  • Updated UserMenu.tsx to use discord credentials when available
  • Updated the next.config.js file to allow discord avatars to be used in the Image tag

website/.env Outdated
@@ -12,3 +12,6 @@ NEXTAUTH_SECRET=O/M2uIbGj+lDD2oyNa8ax4jEOJqCPJzO53UbWShmq98=
EMAIL_SERVER_HOST=localhost
EMAIL_SERVER_PORT=1025
[email protected]

DISCORD_CLIENT_ID=1058355952459452446
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't check these in. Can you remove them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to keep them locally, you can put this in .env.local

Copy link
Contributor Author

@KarthikRaju391 KarthikRaju391 Jan 3, 2023

Choose a reason for hiding this comment

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

Oh I forgot I included it, I will remove and push the changes

Copy link
Collaborator

@bitplane bitplane Jan 3, 2023

Choose a reason for hiding this comment

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

It'll still end up in the git history if you don't rebase to remove the patch.

Warning: git rebase can be dangerous and messy. Back up your branch locally first with git branch some-backup-branch-name so you can get back to it if you mess things up.

Now run git rebase -i main. You can change the order of the commits so the removal commit comes after the first commit on this branch, then change the word pick (select this commit) to be squash (combine this commit with the previous one). This will cause the addition of the secret and the removal to get merged into one patch, magically removing it from the history.

Once you're sure it looks good (git log, git show, git diff main), push it back up with git push -f. You'll need to force push because histories will have diverged.

It'll still exist here on Github though. Not sure about the process for censoring that.

Copy link
Contributor Author

@KarthikRaju391 KarthikRaju391 Jan 3, 2023

Choose a reason for hiding this comment

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

If I do git push -f will that create a new pull request? @bitplane

Copy link
Collaborator

@bitplane bitplane Jan 5, 2023

Choose a reason for hiding this comment

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

@KarthikRaju391
No, it'll just push the rewritten history of your branch. It might mean that some review comments can't link to code that's changed anymore, I'm not sure. There's 3 repos going on, each with a history of commits:

  1. your local repo and the branch where you're working (on your machine it's just called discord-credentials)
  2. Your branch in your github fork KarthikRaju391/Open-Assistant:discord-credentials, which you call origin/discord-credentials locally
  3. This repo's main branch, which is LAION-AI/Open-Assistant:main usually called upstream/main if you added the upstream remote.

Here's what's going on in my instructions

  • git rebase -i will rewrite the history of 1 (discord-credentials)
  • git push -f will overwrite 2 with the contents of 1
  • This pull request is a GitHub thing that requests we pull code from 2 into 3, it's based on the name of 2 not its history

We could do an alternative method: When merging your changes into main, we choose the "squash commits when merging" approach, which will remove all your commits and replace them with a single squashed commit. But that kinda sucks because your commits will be gone, so you as far as GitHub is concerned you won't be credited as one of the project's authors. So the rebase + force push is the best option

@@ -51,6 +51,20 @@ Remember to save your changes.
https://discord.com/oauth2/authorize?client_id=YOUR_CLIENT_ID_HERE&permissions=8&scope=bot%20applications.commands
```

## Discord setup for development
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a different workflow from the "Bot Setup" above? could the two be merged or do they refer to separate applications?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the website part to enable discord authentication

Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't it then be in the README of the website, instead of the one for the discord-bot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we were discussing it in the issue, I was told to put it in discord-bot so, I put it there. I'll just change it

Copy link
Collaborator

@yk yk left a comment

Choose a reason for hiding this comment

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

good from my side 👍 thank you

@KarthikRaju391
Copy link
Contributor Author

Thank you so much for helping me throughout @yk and @fozziethebeat
Will be looking forward to contributing more!

Copy link
Collaborator

@fozziethebeat fozziethebeat left a comment

Choose a reason for hiding this comment

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

Two small changes but I approve of this. I'll merge once I see that the pre-commit checks all pass.

@@ -50,7 +50,6 @@ Remember to save your changes.
```
https://discord.com/oauth2/authorize?client_id=YOUR_CLIENT_ID_HERE&permissions=8&scope=bot%20applications.commands
```

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this line change? That'll simplify the review process

@@ -0,0 +1,6 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you delete this file? It's not needed at the root directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The package-lock.json file should be deleted? @fozziethebeat

@fozziethebeat fozziethebeat merged commit 75e6e72 into LAION-AI:main Jan 5, 2023
@fozziethebeat
Copy link
Collaborator

Thank you for working on this and working through the git changes.

@KarthikRaju391 KarthikRaju391 deleted the discord-credentials branch January 5, 2023 10:07
@KarthikRaju391
Copy link
Contributor Author

@fozziethebeat, it was great working on this! Certainly wouldn't be able to do it without your help.

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

Successfully merging this pull request may close these issues.

4 participants