Feature team_permission, EnvironmentVariable, EnvironmentSecret, and Team extended by team-sync and external-groups

[WolfgangFischerEtas]

Overview of new features:

• team has now team_sync and external_group, this implements syncing with an IdP or external groups defined in an enterprise (where the member are defined
• team permissions are added: they define what team permissions on a repository are available
• environment variables and secrets: they introduce an extension to the current design, instead of only having a parent_object, also a grandparent_object is needed. I decided to not provide a default here, but instead wanted to make really clear where changes are necessary. The grandparent_object is only used in a double nested structure which is necessary for environment: grandparent_object: repository, parent_object:environment and then either variable or secret information. But here a lot of (simple) changes came up.
• tests added for environment variables and secrets, and for team permissions
• documentation added for environment variables and secrets, and for team permissions, also the new attributes for team (team_sync_id, team_sync_name, team_sync_description and external groups)
• in the examples I updated the otterdog-defaults.libsonnet
• the documentation has changed in the range of environment, a new level was introduced for variables and secrets
• the json schemes are extended so that environment variables and secrets, team permissions and the extended team structure are supported
• in jsonnet.py the new functions newTeamPermission, newEnvVariable, newEnvSecret are introduced, together with their default configuration
• to get all the permissions of all teams in an organization I used a double nested graphql call, in the best case only one call is necessary if there are less than 100 repositories with always less than 100 permissions available. Otherwise first another graphql call with the next 100 repositories is done. After that it is checked if there are more than 100 permission settings available if so then the cursor from the first call is used to complete the call for a repository.

If you think the patch is too big I can split it into smaller pieces. Please make a suggestion

[kairoaraujo]

Hi @WolfgangFischerEtas, thanks for this PR.
I'm adding to my list for review

[WolfgangFischerEtas]

Hi @kairoaraujo thanks a lot. I've also corrected the otterdog-defaults.json. I removed the double definition of newTeam. It should now work.

[WolfgangFischerEtas]

Hi @kairoaraujo I merged the lastest commit on my branch and corrected the last mypy issues.

[AlexanderLanin]

@kairoaraujo could you provide some feedback here, as this PR is stuck? Any advice what we can do to support you here?

[netomi]

thanks for the contribution, I will review the latest by EOB today, got busy with other stuff.

[netomi]

the permission values are mixed case here. Is this what the GH API expects / supports?

[WolfgangFischerEtas]

That comes from the fact that I read in the permissions from GraphQL, REST API would consume too many requests, and there the names are READ, WRITE, MAINTAIN, TRIAGE and ADMIN (in uppercase letters), but you cannot update permissions via GraphQL, and in REST API you have to you the lower case equivalents. It's a bit ugly and inconsistent, but I think accepting all these values and map then to the ones REST API accepts is consistent.

If you insist, I can also adapt the description in the context.add_failure call where I just mention the REST API values.

[netomi]

so what we ususally did is to transform data to a canonical form used in the model to avoid confusion or multiple ways to express the same thing. In this case I would transform the data replied by the GraphQL query to the way the Rest API understands it and this I would also use when modelling the configuration via a jsonnet file.

[netomi]

this moves the existing file environment.md from the repository to a new directory which makes sense, but the existing file should be removed afaict.

[netomi]

the settings for secrets and variables are missing afaict.

[netomi]

I tested the environment functionality added as part of the PR and it works fine in general.

A couple of things that I noticed:

• you added the ability to pass a grandparent_object to various method which is needed due to the nested nature of environment secrets and variables, which makes sense, however that is not yet included in the display of the resources, e.g.

Project tinygears[github_id=TinyGearsOrg] (1/1)
  there have been 9 validation infos, enable verbose output to display them.

  - remove env_secret[name="TEST", environment=test-env, repository=tinyunits] {
    - name = "TEST"
  - }

  No changes required.
  1 resource(s) would be deleted with flag '--delete-resources'.

I would expect that also the environment to which this secret belongs shows up in the header like

env_secret[name="TEST", environment=test-env, repository=tinyunits, environment=test-env]

• during testing I noticed that variables and secret in GH are always created uppercase, we are missing a validation for that to make sure that variable and secret names are uppercase as otherwise you get weird effects, like

  - remove env_variable[name="TEST", environment=test-env, repository=tinyunits] {
    - name  = "TEST"
    - value = "myvar"
  - }

  + add env_variable[name="test", environment=test-env, repository=tinyunits] {
    + name  = "test"
    + value = "myvar"
  + }

this is not a fault of the PR, that is already an existing problem, but maybe you can add a validation to secret and variable to make sure the name is all upper case.

[netomi]

The PR is pretty big and includes various new features, the environment stuff alone would have been enough for a single PR. The team permission changes could also be in a separate PR and finally the team sync stuff.

That would make it easier to review and merge the changes, as now its a lot of changes together.

But in general the PR LGTM and follows the existing conventions in the repo as much as possible.

[netomi]

regarding the modelling of the team permissions:

it feels a bit heavy to assign permissions like that:

      team_permissions: [
        orgs.newTeamPermission('developers') {
            permission: 'pull' 
        }
      ],

I would have preferred something like we did for custom_properties:

   team_permissions+: {
      developers: 'write',
      project-managers: 'triage'
   }

see our default config here: https://github.com/EclipseFdn/otterdog-defaults/blob/main/otterdog-defaults.libsonnet#L480-L482

the validation should check if the teams exists afaict. There would be no separate TeamPermission object but that is acceptable imho.

[WolfgangFischerEtas]

Thanks a lot for the review. I will have a look into it next week.

[netomi]

if you could split up this PR in actually 3 separate ones as I outlined above, it will be much easier to merge that afaict.
The content that I reviewed looks good and could be merged quickly.

[WolfgangFischerEtas]

I will do it the following way: 1 for the team, for the team permission according to your proposal, but I will keep this intact. Once the other PRs are merged then this PR gets smaller und clearer. I will rename it then to feat:environment variables and secrets.