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

Add Job to automatically sort the dictionary file if desired. #41

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

acoffman
Copy link
Collaborator

@acoffman acoffman commented Dec 17, 2024

This introduces a new input boolean: sort_dictionary which defaults to false. (I figured it would be best if we didn't start committing to peoples' repositories automatically without them opting into the behavior.)

If the user sets sort_dictionary to true, another job will run when check spelling is run. The job will

  • check out the PR branch
  • ensure that the provided GH_PAT has write permissions to the repo in question
  • sort the dictionary file into a temporary file and check if they differ
  • if so, it deletes the old dictionary file and moves the new one into place, then commits and pushes to the PR branch.

As best as I can tell, it appears that the gh_pat token was not being used the report maker action previously. The way it is currently being invoked (see here) passes the literal string secrets.GH_PAT into report-maker. Changing it to ${{ secrets.GH_PAT }} to get the value directly throws an error because you cannot pass a secret in via an input like that, it needs to be passed in explicitly as a secret . You can see an updated version that functions as expected in the course I've been using the test this here.

If that looks correct to you, I can also create a PR back the OTTR_Template repo with the change. I have a test course repo with a PR demonstrating to new behavior here

I can also go ahead and update the README to document this new option if you're okay with this approach in general.

closes jhudsl/OTTR_Template#233

@acoffman acoffman requested a review from cansavvy December 17, 2024 22:00
Copy link
Contributor

@cansavvy cansavvy left a comment

Choose a reason for hiding this comment

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

@acoffman Thanks for doing this! This all seems reasonable on a first glance. Can you point me to where/how you tested this so I can look into those logs?

@acoffman
Copy link
Collaborator Author

acoffman commented Dec 18, 2024

@cansavvy No problem! I have a repo here cloned from the OTTR Template where I am pointing it to this branch and have an open PR that updates the dictionary: acoffman/test-course#7 (This action run specifically: https://github.com/acoffman/test-course/actions/runs/12381955138/job/34561616152 if that's helpful)

Copy link
Contributor

@cansavvy cansavvy left a comment

Choose a reason for hiding this comment

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

Thanks! This is great!

I just have some questions and ideas that can be implemented elsewhere but to expand on this.

- name: "Check write permissions"
id: check-write-permissions
run: |
sudo apt-get install -y jq
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this is good. We should make this a function and use this elsewhere because permissions (or lack thereof) is a frequent issue for ottr users.

steps:
- name: "Check out PR branch"
id: checkout-pr-branch
uses: actions/checkout@v4
Copy link
Contributor

Choose a reason for hiding this comment

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

So no need for the token argument then?

- name: "Check write permissions"
id: check-write-permissions
run: |
sudo apt-get install -y jq
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add this to the docker image so this can be used everywhere

@cansavvy cansavvy merged commit 8b80d7b into main Jan 8, 2025
4 checks passed
@cansavvy cansavvy deleted the acoffman-ordered-dict branch January 8, 2025 15:58
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.

Make spell-check.R put words in alphabetical order
2 participants