Skip to content

Various tweaks #17

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Various tweaks #17

wants to merge 7 commits into from

Conversation

dehnert
Copy link

@dehnert dehnert commented Jun 11, 2021

I tried setting this up with my own discord server, and made some tweaks in the process. I'm not sure which if any of these would be appealing to you, but just in case.

The changes:

  • Use the GitHub Container registry instead of the Docker registry (provides anonymous access to the images)
  • Publish images from another branch, for testing
  • Logging in tests
  • Bump some dependencies
  • Notes on running integration tests
  • Base sleep time on retention intervals
  • Fix some warnings

dehnert added 7 commits May 30, 2021 14:58
Apparently the GitHub Packages Docker Registry is being replaced by the
Container registry, which among other advantages allows anonymous
access to public images, which seems useful.
When the minimum retention interval supported is an hour, it seems a
little silly to check for messages every minute. When the minimum
retention interval configured is a month, it seems extra silly. 10% is
probably a reasonable error margin in retention interval, and probably
means significantly less time processing.
@bahlo
Copy link
Owner

bahlo commented Jun 13, 2021

Hey, thanks so much for the PR! 🙌 Definitely going to check it out in the next days.

Copy link
Owner

@bahlo bahlo left a comment

Choose a reason for hiding this comment

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

Thanks again for putting the time in! Got two tiny notes and it'd be amazing if you could rebase on main to have CI run on this PR.

if: github.ref == 'refs/heads/container'
run: |
docker build -t ghcr.io/${GITHUB_REPOSITORY}/discord-retention-bot:testing .
docker push ghcr.io/${GITHUB_REPOSITORY}/discord-retention-bot:testing
Copy link
Owner

Choose a reason for hiding this comment

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

Can we remove the whole container branch logic? Does it make sense to build the container on non-main branches but only push it on main instead?

Discord gateway in order to [send the
messages](https://discord.com/developers/docs/resources/channel#create-message)
that the test deletes (otherwise, you'll see a 40001 Unauthorized JSON error).
You can do this by just making a quick script that calls
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
You can do this by just making a quick script that calls
You can do this by making a quick script that calls

Copy link
Owner

Choose a reason for hiding this comment

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

Could we have the script somewhere in the repository to make things easier?

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.

2 participants