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

Upgrade to SourceCred 16 #15

Merged
merged 1 commit into from Sep 27, 2020
Merged

Upgrade to SourceCred 16 #15

merged 1 commit into from Sep 27, 2020

Conversation

teamdandelion
Copy link
Contributor

This also modifies the dependencies.json, so that now we define a start
period, and the Cred minting only kicks in after that period. I set the
initial period to be the next logical Cred period (i.e. starting next
Sunday), so if someone adopts SourceCred today, it will only start
minting dependency Cred for SC in the first week where SC was already
running. We should set up a GitHub action to keep moving the start date
to next Sunday.

Test plan: yarn load; yarn graph; yarn score; yarn site; yarn serve
and verify that there are no errors.

@teamdandelion teamdandelion force-pushed the upgrade-sc-dependencies branch 2 times, most recently from f0b5122 to 34c6a81 Compare September 11, 2020 23:14
@teamdandelion teamdandelion force-pushed the upgrade-sc-dependencies branch 2 times, most recently from 1239242 to 0110c77 Compare September 13, 2020 05:27
@teamdandelion teamdandelion changed the title Upgrade to SourceCred 15 Upgrade to SourceCred 16 Sep 13, 2020
This also modifies the dependencies.json, so that now we define a start
period, and the Cred minting only kicks in after that period. I set the
initial period to be the next logical Cred period (i.e. starting next
Sunday), so if someone adopts SourceCred today, it will only start
minting dependency Cred for SC in the first week where SC was already
running. We should set up a GitHub action to keep moving the start date
to next Sunday.

Test plan: `yarn load; yarn graph; yarn score; yarn site; yarn serve`
and verify that there are no errors.
@@ -2,6 +2,8 @@
{
"autoActivateOnIdentityCreation": true,
"name": "SourceCred",
"startWeight": 0.05
"periods": [
{"startTime": "2020-09-20", "weight": 0.05}
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO it would make more sense to set this when the user first runs the instance instead of trying to keep it up to date with a Github Action. We can do it at the same time we generate the ID for the SourceCred user.

Reasons why:

  • Cleaner commit history (important for users to know how to update their instances with latest changes if we add new config files / features, etc
  • User doesn't end up with that action running in their repo when they fork it
  • Updating it every week would be inaccurate since forking it the day before it updates would mean the start time gets activated in 1 day instead of 1 week.

Copy link
Member

Choose a reason for hiding this comment

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

@HammadJ it seems like you are using this change to make a larger suggestion about the design for interaction between instances and SourceCred itself.

You want to see SourceCred be able to dynamically update an instance so user's can see these changes for themselves in the diff after upgrading their sourceCred dep. I think this is a great idea, but but should this upgraded design prevent this change from merging?

I'm approving because I think we can accommodate this change in the new design you are proposing, and keep with our "incrementalist" philosophy here.

Copy link
Collaborator

@META-DREAMER META-DREAMER left a comment

Choose a reason for hiding this comment

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

IMO we should do this at runtime, not have it hardcoded in the codebase

@@ -2,6 +2,8 @@
{
"autoActivateOnIdentityCreation": true,
"name": "SourceCred",
"startWeight": 0.05
"periods": [
{"startTime": "2020-09-20", "weight": 0.05}
Copy link
Member

Choose a reason for hiding this comment

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

@HammadJ it seems like you are using this change to make a larger suggestion about the design for interaction between instances and SourceCred itself.

You want to see SourceCred be able to dynamically update an instance so user's can see these changes for themselves in the diff after upgrading their sourceCred dep. I think this is a great idea, but but should this upgraded design prevent this change from merging?

I'm approving because I think we can accommodate this change in the new design you are proposing, and keep with our "incrementalist" philosophy here.

@teamdandelion teamdandelion merged commit 203f090 into master Sep 27, 2020
@teamdandelion teamdandelion deleted the upgrade-sc-dependencies branch September 27, 2020 05:38
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.

None yet

3 participants