Skip to content
This repository has been archived by the owner on Nov 6, 2021. It is now read-only.

Add in challenge project configuration #5

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thomasyu888
Copy link
Member

This is the initial draft for creating challenge projects built off of both the imcore and treat-ad templates.

@thomasyu888 thomasyu888 requested a review from a team April 15, 2021 00:54
Copy link

@BrunoGrandePhD BrunoGrandePhD left a comment

Choose a reason for hiding this comment

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

Feel free to push back on any of my suggestions! I just want to have conversations for going one way or another.

Comment on lines +54 to +60
children:
- name: Data
type: Folder
- name: Logs
type: Folder
- name: Resources
type: Folder

Choose a reason for hiding this comment

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

In this template, @thomasyu888 nested each top-level project under a key (e.g. LiveProject, StagingProject). I wonder if we should use the same format for the children attribute for consistency. Alternatively, we could make the top-level elements an array in YAML, but I prefer mappings over arrays (at least aesthetically).

This isn't a hill I'll die on, but I would like to discuss why we would go one way or another.

In the suggestion below, I'm taking advantage of the shortcut where the key is used as the folder name if name isn't given. The point of this is to make the value of children look like the root of the YAML template.

Suggested change
children:
- name: Data
type: Folder
- name: Logs
type: Folder
- name: Resources
type: Folder
children:
Data:
type: Folder
Logs:
type: Folder
Resources:
type: Folder

Copy link
Member Author

@thomasyu888 thomasyu888 Apr 16, 2021

Choose a reason for hiding this comment

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

I like this idea, except the short hand for this is already just:

children:
   - Data
   - Logs

That being said, I can imagine something like this:

children:
    Data:
        type: Folder
        children: ....
    Logs:
	type: Folder

I'd agree with your array strategy. Ill be closing this PR and starting another one in the synapseformation repo

- principal_id: teamA
access_type: *admin
- principal_id: teamB
access_type: *view

Choose a reason for hiding this comment

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

I think this is a good use of anchors and aliases. Maybe we can start with a shared bank of them within the synapseformation repo, including the "canned ACLs" included in this PR.

type: Project
acl:
# principal_id can be both user or team id
- principal_id: teamA

Choose a reason for hiding this comment

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

Should this be called principal_id if it can take principal names? Unless teamA is intended to be the integer ID of the team called teamA on Synapse.

A related question has to do with the output JSON that's been expanded. Should we replace (or expand) any mention of a user/team with the corresponding principal IDs for stability?

Here are some example where I replaced principal_id with principal as one way to address my first question above. I prefer expanding to include both the name and ID for readability, and I would expect SynapseFormation to use the ID over the name if both are present.

# Before
    - principal: teamA
      access_type: *view

# After (replace)
    - principal: 3148189
      access_type: *view

# After (expand)
    - principal: 
        principal_id: 3148189
        principal_name: teamA
      access_type: *view

Copy link

@BrunoGrandePhD BrunoGrandePhD Apr 15, 2021

Choose a reason for hiding this comment

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

Actually, a better approach is to store the expanded team node in the root to avoid duplication and use whatever method we decide on to cross-reference the team (I'm using !Ref here merely for illustrative purposes). The advantage of this approach is that if ever someone wants to change the team (e.g. to another existing team), they only need to change the ID or name in one place rather than every occurrence.

# After (normalize)
    - principal: !Ref teamA
      access_type: *view

[...]

teamA:
  type: Team
  principal_id: 3148189
  principal_name: teamA

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh interesting, so even if the team already exists, explicitly create the team object. Instead of the synapse identifier that's copied, it would just be the key, and the synapse identifier can be changed over time.

Choose a reason for hiding this comment

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

Yeah, reminiscent of database normalization using foreign keys.

This highlights an advantage of using arrays in the root of the templates and under children: instead of mappings (which I prefer aesthetically), namely that synapseformation wouldn't have to figure out a key for each team moved to the root. Copying @mattfazza because this relates to the discussion I spurred in Sage-Bionetworks/synapseformation#27.

I also suggest that we use id and name wherever possible (instead of variations like principal_id). I envision the cross-references could "look through" all of the names in a template to retrieve the associated id. I'm not saying we should implement this now, but opting for this naming convention will make it easier for us down the road to implement cross-references. Food for

# After ()

    - principal: !Ref teamA
      access_type: *view

[...]

- type: Team
  id: 3148189
  name: teamA

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants