Skip to content

bot #551

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 61 commits into
base: main
Choose a base branch
from
Open

bot #551

wants to merge 61 commits into from

Conversation

signorecello
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@critesjosh critesjosh left a comment

Choose a reason for hiding this comment

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

What have you been doing to test this before you deploy updates? I think we should enable CI that at least does some minimal testing

# =============================================================================

# --- Discord ---
bot_token = "${{ secrets.BOT_TOKEN }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am seeing that is best practice to access secrets through shell variables rather than the secrets directly. When accessing secrets directly in run, there is a risk that the secrets get dumped in logs unintentionally.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From what I've seen they get sanitized if printed, but I'll change it to meet best practices


### Local Development with DynamoDB

For local development, you can use DynamoDB Local, which is provided by AWS as a downloadable version that simulates the DynamoDB service.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this copy the database to the local machine?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it just uses a local db. Copying would be a nice thing to do yeah. I'll look into it, I've done it ages ago so I def need a refresher


# Sparta Rollup Tools

This repository contains tools for interacting with the Sparta Rollup, including a subgraph for The Graph and scripts for querying committee membership.
Copy link
Collaborator

Choose a reason for hiding this comment

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

rollup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AI wrote this lol. It's very outdated actually, there's no subgraph anymore and etc. Needs updating

Copy link
Collaborator

Choose a reason for hiding this comment

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

A few questions:

  • Should this page include some docs about the API endpoints?
  • did you add a command that allows approved users to add themselves to the set?
  • Can you add some instructions about the intended flow for someone to update this? If someone on the team wants to add a command, what is the intended workflow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Docs on the API endpoints are autogenerated in /api/docs once you run the service... Not sure if they're needed in the readme. Same for adding themselves to the set, you can just use the API page (swagger).

But for the instructions yeah I can add some instructions, this is terraform and everything runs correctly with just terraform apply --var-file <var-file> but it assumes whoever is updating it understands basics on terraform

@signorecello
Copy link
Collaborator Author

No testing for now 😬 the reason I haven't invested too much in tests is just that requirements change so much, I'd just be having double trouble. But it may just be the time to add some proper testing yeah

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