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

Updates to Config Generation #58

Merged
merged 11 commits into from
Feb 2, 2024

Conversation

Abby-Wheelis
Copy link
Member

In testing before our demo today, I realized that the workflow was still committing node-modules to the PR branch.

I recognized that this was happening in the pull-request-generation step of the workflow, and looked into the documentation for that process.

I needed to specify an 'add-path' parameter to restrict the tracked changes to the configs folder.

I plan to go back and see if there are any other review comments from previous iterations that I can also address here.

Abby Wheelis added 2 commits January 25, 2024 11:08
on a recent test, all the depedencies got committed, again

We ONLY want changes in the config folder, so we must specify the add-paths parameter
updating naming conditions to prevent duplication and keep track of the issue number
@Abby-Wheelis
Copy link
Member Author

These two screenshots show that, when running on my main branch, only one file is committed, and that there are no node-modules files on my branch, which the accidental presence of those files previously was what prevented me from noticing that all files were being committed, since they were already on my branch.

Screenshot 2024-01-25 at 11 16 39 AM
Screenshot 2024-01-25 at 11 17 09 AM

@Abby-Wheelis
Copy link
Member Author

Abby-Wheelis commented Jan 25, 2024

Before I forget what else I want to do here:

  • ensure leading/trailing whitespace gets trimmed from the admin emails so that doesn't create issues downstream
  • limit the size of the list of admin emails to 3? 4?

With the list size limitations, it would be ideal if I could do this as a constraint of the form, but I'm not sure if that's supported. I can absolutely document in the form that there's a limit, but if I constrain it in process that would lead to a process failure and no created pull request ... is that something we're ok with?

Abby Wheelis added 2 commits January 26, 2024 10:46
strip off the leading and trailing whitespace so the emails are valid

set a failure when the list is too long
rename to email_list (admin_list was duplicated) and actually instantiate the variable so the process works
@Abby-Wheelis
Copy link
Member Author

Sucessfully shortened the list to a max of 4 emails (but we could change this) - an error message will show in the action tab if it fails, I still need to add a warning to the form itself
Screenshot 2024-01-26 at 11 08 09 AM

@Abby-Wheelis
Copy link
Member Author

Testing done: 1 form with too many emails, job failed with error message. 1 form with 3 emails, extra leading and trailing whitespace verified as gone.

Screenshot 2024-01-26 at 11 18 44 AM Screenshot 2024-01-26 at 11 18 55 AM

All pull requests only comitting the config file, verified by ensuring no node_modules files in repo before running the job, and only one file (config file) added to generated pull request

based on review comment - e-mission#49 (comment)
@Abby-Wheelis Abby-Wheelis marked this pull request as ready for review January 26, 2024 18:28
@Abby-Wheelis
Copy link
Member Author

I have now tested the lowercase functionality!
Screenshot 2024-02-01 at 5 14 39 PM

Screenshot 2024-02-01 at 5 14 29 PM

@shankari
Copy link
Contributor

shankari commented Feb 2, 2024

At this point, you are auto-correcting errors. I am merging this now, but think that a better approach might be to fail the process, flag the errors and let them fix them.

I wonder if we should also just create a website instead of the issue form, but I am not sure of the best way to embed the GitHub token so that we can create the PR. @nataliejschultz I know you have used AWS secrets in a website; would that be an option to store a GitHub access key?

@shankari shankari merged commit 0007204 into e-mission:main Feb 2, 2024
@nataliejschultz
Copy link
Contributor

I wonder if we should also just create a website instead of the issue form, but I am not sure of the best way to embed the GitHub token so that we can create the PR. @nataliejschultz I know you have used AWS secrets in a website; would that be an option to store a GitHub access key?

Yes, you can store the token in secrets manager.
However, does the token change frequently? If so, there needs to be an automated process to update the secret with the new token. It also depends on where the website is hosted. We need to be able to assume an IAM role that has access to our token secret in Secrets Manager to retrieve the token and use it to create a PR.

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