repository-settings / app

Pull Requests for GitHub repository settings
https://github.com/apps/settings
ISC License
922 stars 177 forks source link

Support variables in environments #711

Open rlmartin opened 1 year ago

rlmartin commented 1 year ago

Prerequisites:

New Feature

Please describe the desired new functionality: Under the environments section, add a vars section that is a map of var name => var value. Example:

environments:
  - name: production
    vars:
      AWS_DEPLOYMENT_ROLE: arn:aws:iam::0000000000:role/deployment-role
  - name: development
    vars:
      AWS_DEPLOYMENT_ROLE: arn:aws:iam::1111111111:role/deployment-role
travi commented 1 year ago

i'm supportive of this addition. i would like to see us avoid abbreviations, so i'd prefer variables over vars, but this otherwise seems reasonable. PRs welcome. I will be stepping away for a couple of weeks, but will catch up afterward.

ctoestreich commented 7 months ago

Any movement on this?

ctoestreich commented 7 months ago

@rlmartin I took a stab at getting the support in place, but the ability to add environment varaibles required the repository id not the name so I had to add an extra call to get this.

@travi It does not look like the repository id is used anywhere else and the context for what is handed into the app is unclear if this is already on the this.repo object as something like this.repo.id. If that is the case it simplifies the code a lot and I will not have to make the additional call to get the id.

If the call is needed then I need a bit of help fixing the mock as my jest skills are a bit rusty and I can't seem to figure out why the mock to the get repository isn't working. 🤷

travi commented 6 months ago

sorry for the delay responding on this one. i was working on refactoring the integration tests and upgrading to the latest probot version, so i didnt want my feedback to change after working through that effort.

the ability to add environment varaibles required the repository id not the name so I had to add an extra call to get this

can you point to documentation related to this? this doesnt align with what i'm finding. for example, this doc highlights that the repository name is needed, not the id: https://docs.github.com/en/rest/actions/variables?apiVersion=2022-11-28#create-an-environment-variable

the context for what is handed into the app is unclear if this is already on the this.repo object as something like this.repo.id.

the repo data populated in this.repo comes from the webhook payload that is received in our listeners. https://github.com/octokit/webhooks/ is a good reference for the expected content of these payloads. for example, here is an example of how repository is represented in a push webhook.

I need a bit of help fixing the mock as my jest skills are a bit rusty and I can't seem to figure out why the mock to the get repository isn't working.

i agree that the previous testing approach was a bit awkward, so i used the upgrade to the latest probot version as an excuse to rework the approach for integration testing. use of cucumber enabled making the scenarios a lot easier to describe and understand when approaching the project without the whole thing already in your head. hopefully this helps make things more clear, but let me know if additional guidance would be helpful