Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add node-engine to snapcraft.yaml #509
Conversation
earnubs
and others
added some commits
May 20, 2016
sergiusens
reviewed
May 23, 2016
| @@ -64,6 +63,9 @@ def schema(cls): | ||
| }, | ||
| 'default': [], | ||
| } | ||
| + schema['properties']['node-engine'] = { | ||
| + 'type': 'string' |
earnubs
May 23, 2016
•
Edited 1 time
-
earnubs
May 23, 2016
Contributor
-
May 23, 2016
Originally I did have the default, but I'm not sure it's exactly what you want because you're almost always going to have a default that is out of date, as such I thought it better to make the developer take full control of this, and no default...
sergiusens
May 24, 2016
Collaborator
Yes, but this breaks backwards compatibility so we now are stuck with it. We update snapcraft often enough that we can always keep it using the best choice available
sergiusens
reviewed
May 23, 2016
| + self._nodejs_tar = sources.Tar( | ||
| + _get_nodejs_release(self.options.node_engine), | ||
| + os.path.join(self.partdir, 'npm')) | ||
| + except AttributeError: |
sergiusens
May 23, 2016
Collaborator
by adding a default there is no need for this exception dance ;-)
kyrofa
reviewed
May 23, 2016
| + _get_nodejs_release(self.options.node_engine), | ||
| + os.path.join(self.partdir, 'npm')) | ||
| + except AttributeError: | ||
| + raise AttributeError('node-engine not defined in snapcraft.yaml') |
kyrofa
May 23, 2016
Member
This probably means it needs to be required in the schema instead of deleting the required key on line 71.
kyrofa
May 23, 2016
•
Edited 1 time
-
kyrofa
May 23, 2016
Member
-
May 23, 2016
Not you, but the plugin currently deletes it on line 71. If you add the node-engine option to the required key, snapcraft will catch it when validating the YAML instead of your needing to do it here. See the copy plugin for an example of what you'll probably need to do.
sergiusens
reviewed
May 23, 2016
| @@ -64,6 +63,9 @@ def schema(cls): | ||
| }, | ||
| 'default': [], | ||
| } | ||
| + schema['properties']['node-engine'] = { |
added some commits
May 23, 2016
sergiusens
reviewed
May 24, 2016
| + # `node-engine` keyword is required, but `node-packages` is not. | ||
| + schema['required'].append('node-engine') | ||
| + schema['required'].remove('source') | ||
| + schema['properties']['source']['default'] = '.' |
sergiusens
May 24, 2016
Collaborator
Can we leave this one for later? default is a contentious topic and would be better applied across the board
|
Thanks for this, looks good. Mind rebasing or mergin master in to get rid of whatever the conflict github mentions is? |
snappy-m-o
commented
May 24, 2016
|
Can one of the admins verify this patch? |
kyrofa
reviewed
May 24, 2016
| + # `node-engine` keyword is required, but `node-packages` is not. | ||
| + schema['required'].append('node-engine') | ||
| + schema['required'].remove('source') | ||
| + schema['properties']['source']['default'] = '.' |
kyrofa
May 24, 2016
Member
I doubt you want this line here (defaulting the source to '.'). Also, the comment doesn't seem quite right. Perhaps:
# `node-engine` keyword is required, but `source` is not.
schema['required'].append('node-engine')
schema['required'].remove('source')
and others
added some commits
May 25, 2016
|
OK, I am |
|
@earnubs mind rebasing please? I think that's why the diff looks odd, and that was our mistake, not yours-- sorry about that |
|
@kyrofa for reasons I don't understand I cannot rebase this without getting stuck in an endless merge conflict loop :) I've merged master and that seems to have cleared up the PR diff, but if you're not happy I'll submit a new PR (rather than figure out what's going on with this rebase). |
|
@earnubs ah! That works just fine, thank you |
|
ok to test |
|
ok to test |
kyrofa
merged commit d579b55
into
snapcore:master
May 26, 2016
added a commit
to josepht/snapcraft
that referenced
this pull request
May 31, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
earnubs commentedMay 23, 2016
•
Edited 1 time
-
earnubs
May 23, 2016
Allow the developer to set the version of node they want, the current plugin uses 4.2.2, LTS is 4.4.4 and higher versions enable more of ES2015 http://node.green/