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.

Sign up for GitHub

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

Jump to bottom

Closes #78 Add support to specify seeds to be created at server start up #80

Merged
merged 8 commits into from
Apr 3, 2018
Merged

Closes #78 Add support to specify seeds to be created at server start up #80

merged 8 commits into from
Apr 3, 2018

Conversation

kecso
Copy link
Member

@kecso kecso commented Apr 3, 2018 •
edited by pmeijer
Loading

Introduces the config option seedProjects.createAtStartup an array of projects to be created at startup.

{
  seedId: 'EmptyProject',
  projectName: 'StartProject',
  creatorId: 'siteAdminOrAnAdminInOwnerOrg', // If not given the creator will be the authentication.adminAccount
  ownerId: 'MyPublicOrg' // If not given will be the creator
  rights: {
    //owner gets all rights by default
    guest: { read: true }
  }
}

@kecso kecso added this to the v2.19.7 milestone Apr 3, 2018
@kecso kecso requested a review from pmeijer April 3, 2018 03:47
pmeijer
pmeijer requested changes Apr 3, 2018
View reviewed changes
Copy link
Contributor

@pmeijer pmeijer left a comment

Choose a reason for hiding this comment

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

It looks good with good coverage. I have some comments - some minor some are pretty crucial.

src/utils.js Outdated
projectsToCreate = [];


Q.ninvoke(storage, 'getProjects', {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't storage.getProjects already return a promise? More importantly the user isn't specified and would only return guest's projects..

Copy link
Contributor

Choose a reason for hiding this comment

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

The user should be the creator (as it would have access to the owners projects too).

src/utils.js Outdated
existingProjectIds.push(projectInfo._id);
});

(gmeConfig.seedProjects.createAtStartup || []).forEach(function (projectInfo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You already check in the validator that this is an array..

src/utils.js Outdated
}


return Q.ninvoke(gmeAuth, 'listUsers', null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again - this returns a promise...

src/utils.js Outdated
logger.error('Cannot create project [' + projectInfo.id + '] as creator [' +
projectInfo.creatorId + '] is missing!');
projectInfo.failed = true;
promises.push(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does null really work well with Q.all ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nm, it looks like the tests covers that 👍
However I think an error here should fail and halt the startup process..

test/server/standalone.startup.spec.js Outdated
.nodeify(done);
});

it('should not create startup projects that has bad creator assigned to it', function (done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this resolve with error?

pmeijer
pmeijer reviewed Apr 3, 2018
View reviewed changes
src/utils.js Outdated

tokens = tokens_;
creators.forEach(function (owner) {
promises.push(storage.getProjects({user: owner}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to write a test that catches this.. It still uses the guest account - it should be username..

Copy link
Member Author

Choose a reason for hiding this comment

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

cr.p

pmeijer
pmeijer requested changes Apr 3, 2018
View reviewed changes
Copy link
Contributor

@pmeijer pmeijer left a comment

Choose a reason for hiding this comment

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

You're not passing the correct argument to getProjects. This should be caught by a test

@pmeijer pmeijer merged commit 95fdb36 into master Apr 3, 2018
@pmeijer pmeijer deleted the startup_seeds branch April 4, 2018 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmeijer pmeijer pmeijer approved these changes

Assignees

@pmeijer pmeijer

Labels
Enhancement Minor Server
Projects
None yet
Milestone
v2.19.7
Development

Successfully merging this pull request may close these issues.
2 participants
@kecso @pmeijer