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

docs: fix typo in server.py #471

Merged
merged 1 commit into from
Jun 12, 2024
Merged

docs: fix typo in server.py #471

merged 1 commit into from
Jun 12, 2024

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Jun 7, 2024

Description (e.g. "Related to ...", etc.)

Please replace this description with a concise description of this Pull Request.

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly

Automated linters

You can run the lints that are run on CI locally with:

poetry install --all-extras --with dev
poetry run poe lint

@alcarney
Copy link
Collaborator

@tombh do you think we could remove the Required constraint on the pipelines?

We wouldn't merge with failing check(s) anyway and it seems to be preventing simple PRs like this from being merged...

@tombh
Copy link
Collaborator

tombh commented Jun 12, 2024

Are you talking about the "Approve To Run" button for PRs from public forks? https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

The idea is that it prevents potential credential stealing. If anybody can trigger a CI run, then they could also submit code that collected all the private ENV variables and secrets used in CI. So approving each CI run is just an extra security layer.

I think this PR just needs to updated from main?

@tombh tombh merged commit da9b77b into openlawlibrary:main Jun 12, 2024
16 checks passed
@Viicos Viicos deleted the patch-2 branch June 12, 2024 06:11
@alcarney
Copy link
Collaborator

Are you talking about the "Approve To Run" button for PRs from public forks?

No, after that, once we've clicked "Approve" and GitHub starts spinning up all of the workflows.

image

This screenshot is from #465, since there are no code changes most of the workflows are (quite rightly) skipped. However, since they are marked as Required criteria for the PR to be merged the merge button will not be enabled.

So, I'm wondering if it is worth us removing the Required flag from each of the workflows? I trust us to not merge when there is a failing check :)

@tombh
Copy link
Collaborator

tombh commented Jun 14, 2024

I'm pretty sure #465 is not running CI because the branch is out of date with main branch. It's because of this repo setting:
image

The idea being that if PRs were to only receive updates from main at the moment of merge (this is theoretically possible when Git can do a so-called "fast-forward" merge) then the CI hasn't actually truly tested whether the PR doesn't introduce regressions.

@alcarney
Copy link
Collaborator

Ah ok, I think I'd assumed CI would run even if the branch was behind.

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.

3 participants