FLUID-5799, FLUID-5967: Reducing Fragility #5

jobara · 2016-08-11T16:38:52Z

https://issues.fluidproject.org/browse/FLUID-5799

Addresses the three areas of issues as indicated in FLUID-5799

Option to change the remote it uses to push to
Provide hints when a task has an error
Echos the commands being executed to the console

Also updated to ESLint

Added an option to retrieve the module's own version.
https://issues.fluidproject.org/browse/FLUID-5967

Will check that the specified remote actually exists, and throw an error if it doesn't. Also added options to specify the remote name.

Updated the tests and README

jobara · 2016-08-11T16:40:51Z

Bumped the version to 2.0.0 because of backwards incompatible changes.

jobara · 2016-08-11T16:50:29Z

@amb26 also I noticed when switching to ESLint that it seems to run against the files in the node_modules directory. Everything seems to be passing; which makes me feel lucky or perhaps it isn't running against those files, but is still adding them to the count.

amb26 · 2016-08-24T15:18:09Z

publish.js

 "use strict";

 var publish = {};
 var path = require("path");
 var extend = require("extend");

+// TODO: The supported version of node.js does not yet support ES6 template strings
+// When version node.js 4.x.x is supported this can be replaced by native support.
+var es6Template = require("es6-template-strings");


I don't see why we can't make node 4.x, the current LTS, the minimum supported version for this project. I don't believe anyone in our communities is working with any earlier version

@amb26 I agree we should be able to set the minimum to 4.0.0. However, while exploring ES6 template literals. ( https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Template_literals ) it is required that the literal use "`". These back ticks fail our JSON linting. I can't actually find anywhere that indicates that they should be valid or not in JSON. Any thoughts?

Leaving in es6-template-strings based on our conversation in the channel.
https://botbot.me/freenode/fluid-work/2016-08-30/?msg=72191253&page=1

jobara · 2016-08-30T13:23:25Z

@amb26 ready for more review

programmatically and via a command line flag.

amb26 · 2016-10-03T01:11:48Z

.eslintrc.json

@@ -0,0 +1,80 @@
+{


Please could you update this to our new shared eslint repo - thanks!

amb26 · 2016-10-03T01:20:13Z

README.md

@@ -127,6 +141,17 @@ publish.standard();
        </tr>
        <tr>
            <td>
+                <code>checkRemoteCmd</code>


This material doesn't appear in this diff (yet) but this is the closest line:

i) There's a typo where "--standard" appears twice, presumably the first instance should be "--version"
ii) The heading named "Node" is pretty confusing and it's unclear what it refers to. There should be some text here to explain what is being documented
iii) There then seems to be some kind of missing heading since the docs then continue with "parameters" again which is a duplicate.
iv) The JSON-encoded form for accepting "options" will be pretty hard for users to use. We should instead accept standard UNIX-style key-value pairs which will be applied to the options individually - for example,

fluid-publish remoteName="origin" --standard

etc.

Instead of being passed in as a stringified options object, each individual option is now taken in as a key/value pair.

jobara · 2016-10-03T14:47:08Z

@amb26 updated to sharable eslint configuration, cleaned up README, and changed command line options to be key/value pairs as you recommended.

amb26 · 2016-10-24T17:18:26Z

We should update this pull so that it executes any "prepublish" script registered in package.json before publishing to npm, now that we are making use of this step within Infusion with fluid-project/infusion#760

jobara · 2016-10-24T17:22:34Z

@amb26 i thought that would just happen automatically when we tried to do an NPM publish.

amb26 · 2016-10-24T18:34:28Z

@jobara I guess that's right - sorry for not thinking too clearly. I had forgotten how the workflow of this utility works.

amb26 · 2016-11-30T20:01:41Z

publish.js

@@ -39,20 +44,15 @@ publish.getPkg = function (moduleRoot) {
    return require(modulePkgPath);
 };

-var pkg = publish.getPkg(__dirname);
-var defaults = pkg.defaultOptions;
-
 /**
 * Processes the argv command line arguments into an object
 *
 * @returns {Object} - the CLI arguments as an options object
 */
 publish.getCLIOpts = function () {


We should add some docs here to explain what format the options are expected to be in - e.g. that they must be in units which contain no spaces, with an "=" character separating key from value, with "false" interpreted as a boolean, and an absence of "=" indicating true.
We should also fix up the irregularity that doesn't specially interpret "true" even though we interpret "false".

amb26 · 2016-11-30T20:02:21Z

publish.js

+ */
+publish.execSyncFromTemplate = function (cmdTemplate, cmdValues, hint, isTest) {
+    var cmdStr = es6Template(cmdTemplate, cmdValues);
+    publish.log("Executing Command: " + cmdStr + "\n");


I think the "\n" character is unnecessary here since console.log already starts a new line

amb26 · 2016-11-30T20:08:02Z

publish.js

@@ -287,7 +304,8 @@ publish.clean = function (moduleRoot, options) {
 * @param options {Object} - see defaultOptions in package.json for possible values
 */
 publish.dev = function (isTest, options) {
-    var opts = extend(true, {}, defaults, options);
+    var publishPkg = publish.getPkg(__dirname);
+    var opts = extend(true, {}, publishPkg.defaultOptions, options);


We should make publish.dev more robust to ensure that it does cleanup if something goes wrong - for example, if npm publish fails, the working copy is left dirty with the modified package.json. We should make sure that we always run publish.clean even in the event of a failure.

The cleanup will happen even in cases where the publishing or tagging steps fail. This way the repo won't be left in a "dirty" state.

The getCLIOpts method now converts both "true" and "false" to their respective booleans.

jobara · 2016-12-08T21:55:15Z

@amb26 ready for more review

amb26 · 2016-12-19T16:13:10Z

As discussed in #fluid-work today, we should fix our npm publish step to avoid polluting the "latest" dist tag: https://botbot.me/freenode/fluid-work/2016-12-19/?msg=78159639&page=1

jobara · 2016-12-20T14:42:48Z

@amb26 thanks for catching the issue with tagging dev releases. I've implemented a fix and added it to this PR. Ready for more review.

amb26 · 2017-01-05T17:21:56Z

README.md

+                <code>publishDevHint</code>
+            </td>
+            <td>
+                A hint for addressing an issue where publishing to the registry fails.


Description should distinguish from publishHint

amb26 · 2017-01-05T17:22:51Z

README.md

+                A hint for addressing an issue where publishing to the registry fails.
+            </td>
+            <td>
+                "Ensure that you have access to publish to the registry and that the current version does not already exist.\nIf the tag already exists use a new tag name or run \"npm dist-tag rm ${packageName} ${tag}\" to remove the existing one.\nAlso ensure that the tag name is valid (i.e. doesn't conform to a valid semver range like v1.4 or 1.4).\n"


The last sentence is puzzlingly worded - is it the case that a tag name CANNOT correspond to a semver version, or is it the opposite?

amb26 · 2017-01-05T17:23:34Z

publish.js

-        var publishCmd = options.publishCmd || defaults.publishCmd;
-        publish.execSync(publishCmd);
+        var pubCmd = isDev ? options.publishDevCmd : options.publishCmd;
+        publish.execSyncFromTemplate(pubCmd, options, options.publishHint);


It looks like this command doesn't use options.publishDevHint when publishing a dev release, as I imagine it should

amb26 · 2017-01-05T17:30:02Z

tests/publishTests.js

@@ -318,81 +318,68 @@ console.log("\n*** publish.pubImpl ***");

 var pubImplFixture = [{


A small strategy hint comment would help the reader here - something like 'In the cases where commands should not execute, their entry in the structure has been filled with a placeholder string beginning "shouldn't"'

amb26 · 2017-01-05T17:34:16Z

package.json

    "grunt-jsonlint": "1.0.4",
-    "sinon": "^1.17.1"
+    "sinon": "1.17.1"
  },
  "defaultOptions": {


Note for the future - this strategy of packing configuration options into package.json is cute, but doesn't actually enable reuse of the functionality. In order to customise these strings, the user has to fork this module and change them! When we start work on the "mature multirepo fluid-publish" sketched out in #7, we should move this module over to being a standard Infusion component and have all these options as part of its defaults.

Yes, the default values aren't easily modifiable. In execution they are, but I agree that's not great for reuse and building upon.

jobara · 2017-01-12T20:19:13Z

@amb26 ready for more review.

amb26 · 2017-01-20T17:16:15Z

package.json

+    "changesHint": "Address uncommitted changes: Commit \"git commit -a\", Stash \"git stash\" or Clean \"git reset --hard\"\n",
+    "checkRemoteHint": "Run \"git remote -v\" for a list of available remote repositories.\n",
+    "publishHint": "Ensure that you have access to publish to the registry and that the current version does not already exist.\n",
+    "publishDevHint": "Ensure that you have access to publish to the registry and that the current version does not already exist.\nIf the tag already exists use a new tag name or run \"npm dist-tag rm ${packageName} ${tag}\" to remove the existing one.\nTags share a namespace with versions. To reduce confusion, tags that can be interpreted as semver values, will be rejected.\n",


I still don't understand what the last two sentences of this hint are trying to explain. When it says, "will be rejected", who is going to do the rejecting? I don't see that you have implemented any behaviour to implement this rejecting. Also, isn't the point of publishing dev releases that the system automatically computes the version number, and does not do any tagging? Perhaps this comment is talking about npm tags rather than git tags. But I thought that all we did with these was to apply "dist" to the latest full release, not to dev releases. I am completely confused! Can you reduce the confusion further :)

@amb26 yes, this refers to NPM tags, not version control tags. The command used is publishDevCmd": "npm publish --tag ${devTag} and devTag defaults to "dev". However, if a user were to override the dev tag to say "v2.2" or something of that sort, it would be rejected by NPM. So the rejection is not happening by fluid-publish but by NPM.

jobara · 2017-01-24T17:14:17Z

@amb26 ready for more review.

jobara added 5 commits November 27, 2015 10:29

FLUID-5799: Check existence of remote repo

c863229

Will check that the specified remote actually exists, and throw an error if it doesn't. Also added options to specify the remote name.

FLUID-5799: Refactored to log all exec commands

d8bf7b2

FLUID-5799: Providing hints when errors occur.

a64ac5f

Updated the tests and README

NOJIRA: Updated to ESLint and linted code

4a37de5

FLUID-5799: Version bump

2edf5c8

jobara assigned amb26 Aug 11, 2016

NOJIRA: Updated fluid-grunt-eslint to fix count of linted files

bf764ca

amb26 reviewed Aug 24, 2016
View reviewed changes

FLUID-5799: Bumping node requirement to 4.0.0

140c2cd

jobara added 2 commits August 31, 2016 08:46

NOJIRA: Updating Linting rules

0a69275

FLUID-5967: added an option to get the module's version

be30428

programmatically and via a command line flag.

jobara changed the title ~~FLUID-5799: Reducing Fragility~~ FLUID-5799, FLUID-5967: Reducing Fragility Sep 22, 2016

NOJIRA: Updating grunt dependency

49e5978

jobara added this to the Infusion 2.0 Release milestone Sep 27, 2016

amb26 reviewed Oct 3, 2016

View reviewed changes

.eslintrc.json

@@ -0,0 +1,80 @@

{

Copy link

Member

amb26 Oct 3, 2016

Choose a reason for hiding this comment

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

Please could you update this to our new shared eslint repo - thanks!

amb26 reviewed Oct 3, 2016

View reviewed changes

jobara added 3 commits October 3, 2016 10:01

NOJIRA: Updated to sharable eslint config

36feebf

FLUID-5799: Cleaning up README

cd241fc

FLUID-5799: Command line options refactored

cb85d89

Instead of being passed in as a stringified options object, each individual option is now taken in as a key/value pair.

amb26 mentioned this pull request Oct 24, 2016

FLUID-5978: Infusion build distributions fluid-project/infusion#760

Merged

amb26 reviewed Nov 30, 2016

View reviewed changes

jobara added 4 commits December 8, 2016 16:21

FLUID-5799: Removed "\n" from log statement

099f773

FLUID-5799: Fixed unit tests

0e0d366

FLUID-5799: Cleanup after updating version in dev builds

7a4f85a

The cleanup will happen even in cases where the publishing or tagging steps fail. This way the repo won't be left in a "dirty" state.

FLUID-5799: Refactoring getCLIOpts and updating related docs

74478a1

The getCLIOpts method now converts both "true" and "false" to their respective booleans.

FLUID-5799: Fixing bug with tagging dev releases

66d5a98

amb26 reviewed Jan 5, 2017

View reviewed changes

jobara added 4 commits January 12, 2017 11:23

FLUID-5799: Updating hint descriptions

2775e6a

FLUID-5799: Clarifying tag requirements for dev releases.

3ed2231

FLUID-5799: using publishDevHint for dev releases

e663e25

FLUID-5799: Updated test comment

35a2d3c

amb26 reviewed Jan 20, 2017

View reviewed changes

FLUID-5799: More clarifications to the publishDevHint

f7da24a

amb26 merged commit f7da24a into fluid-project:master Jan 24, 2017

jobara deleted the FLUID-5799 branch March 15, 2018 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FLUID-5799, FLUID-5967: Reducing Fragility #5

FLUID-5799, FLUID-5967: Reducing Fragility #5

jobara commented Aug 11, 2016 •

edited

jobara commented Aug 11, 2016

jobara commented Aug 11, 2016

amb26 Aug 24, 2016

jobara Aug 29, 2016 •

edited

jobara Aug 30, 2016

jobara commented Aug 30, 2016

amb26 Oct 3, 2016

amb26 Oct 3, 2016

jobara commented Oct 3, 2016

amb26 commented Oct 24, 2016

jobara commented Oct 24, 2016

amb26 commented Oct 24, 2016

amb26 Nov 30, 2016

amb26 Nov 30, 2016

amb26 Nov 30, 2016

jobara commented Dec 8, 2016

amb26 commented Dec 19, 2016

jobara commented Dec 20, 2016

amb26 Jan 5, 2017

amb26 Jan 5, 2017

amb26 Jan 5, 2017

amb26 Jan 5, 2017

amb26 Jan 5, 2017

jobara Jan 12, 2017

jobara commented Jan 12, 2017

amb26 Jan 20, 2017

jobara Jan 23, 2017

jobara commented Jan 24, 2017

		@@ -318,81 +318,68 @@ console.log("\n* publish.pubImpl *");

		var pubImplFixture = [{

FLUID-5799, FLUID-5967: Reducing Fragility #5

FLUID-5799, FLUID-5967: Reducing Fragility #5

Conversation

jobara commented Aug 11, 2016 • edited

jobara commented Aug 11, 2016

jobara commented Aug 11, 2016

Choose a reason for hiding this comment

jobara Aug 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobara commented Aug 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobara commented Oct 3, 2016

amb26 commented Oct 24, 2016

jobara commented Oct 24, 2016

amb26 commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobara commented Dec 8, 2016

amb26 commented Dec 19, 2016

jobara commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobara commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobara commented Jan 24, 2017

jobara commented Aug 11, 2016 •

edited

jobara Aug 29, 2016 •

edited