Development Tooling #377

htdvisser · 2019-03-28T09:56:31Z

Summary:

This PR migrates our development tooling to Mage.

Closes #3
Closes #11
Closes #66
Closes #34

Changes:

Rewrote Git hooks to Mage
Rewrote headers check to Mage (and removed problematic fix/delete)
Rewrote Go tooling to Mage
Rewrote JS tooling to Mage
Rewrote most of the proto tooling to Mage
Updated Babel and some related fixes

Notes for Reviewers:

There are a lot of fixup commits in here (although they aren't indicated with !fixup), but unfortunately rebasing all of this isn't feasible.

The fix-grpc-gateway-names.sh script is still TODO. @rvolosatovs promised to solve that in a future PR

Release Notes

Updated the development tooling. If you are a developer, make sure to check the changes in CONTRIBUTING.md and DEVELOPMENT.md.

Too many changes since last review

kschiffer · 2019-04-04T12:29:38Z

This now also closes #34.

bafonins

@kschiffer please review the list of babel plugins you added, lets try to add less stage 0 proposals. Also, what about the .babelrc file?

$ make js.clean
make: *** No rule to make target `js.clean'.  Stop.

bafonins · 2019-04-05T05:54:59Z

package.json

+        }
+      ],
+      "@babel/plugin-proposal-nullish-coalescing-operator",
+      "@babel/plugin-proposal-do-expressions",


do we need this?

bafonins · 2019-04-05T05:56:10Z

package.json

+      ],
+      "@babel/plugin-proposal-nullish-coalescing-operator",
+      "@babel/plugin-proposal-do-expressions",
+      "@babel/plugin-proposal-function-bind"


do we really want to import stage 0 features?

We had them enabled before, that's why they're added now individually. I can also remove some, as it's true that we actually don't use most of them.

bafonins · 2019-04-05T05:57:31Z

pkg/webui/console/api/stubs/fake-data.js


    return {
      ids: {
        device_id: faker.lorem.slug(),
        application_ids: {
-          application_id: app.ids.application_id,
+          application_id: '',


how is this related to this PR?

After the babel 7 upgrade, this line resulted in an error. Didn't want to bother a lot about it, as it is dead code anyway.

bafonins · 2019-04-05T06:00:33Z

.mage/js.go

+	return eslint("./pkg/webui/**/*.snap", "--no-ignore", "--color")
+}
+
+func (js Js) LintAll() {


missing comment

kschiffer · 2019-04-05T08:32:53Z

This now also closes #66.

rvolosatovs · 2019-04-29T08:11:09Z

.mage/proto.go

+
+// All generates protos.
+func (p Proto) All(ctx context.Context) {
+	mg.CtxDeps(ctx, Proto.Image)


Remove this.

It's redundant, since docker run will automatically pull the image if not available.

It breaks proto:all in offline scenarios in cases where the image is already available and in cases where image is available locally, but not in the repository.

This target generates protos. Updating the image is an unexpected side-effect.

When I ran this proto:all without pulling the image first, it started pulling the image concurrently for all the jobs below. We don't want that. Feel free to suggest an alternative.

I favor offline support over concurrent pulling images. If we can have (explicit) offline support, I'm fine with that too, but we need offline support.

Fixed that in 82c670f

rvolosatovs · 2019-04-29T08:25:57Z

CONTRIBUTING.md

@@ -245,7 +245,7 @@ In the API, the enum descriptions, error messages and event descriptions availab
 These messages are then collected in the `config/messages.json` file, which will be processed in the frontend build process, but may also be used by other (native) user interfaces. When you define new enums, errors or events or when you change them, the messages need to be updated into the `config/messages.json` file.

 ```sh
-make messages
+make go.messages


Why not go:messages?

rvolosatovs · 2019-04-29T08:26:24Z

DEVELOPMENT.md

@@ -142,7 +143,7 @@ From the `.proto` files, we generate code using the `protoc` compiler. As we pla
 The actual commands for compilation are handled by our Makefile, so the only thing you have to execute, is:

 ```sh
-make protos.clean protos
+make proto.clean proto.all


Why not proto:clean and proto:all ?

rvolosatovs · 2019-04-29T08:27:20Z

Makefile

+		$(err) "Previous operations have created changes that were not recorded in the repository. Please make those changes on your local machine before pushing them to the repository:"; \
+		git diff; \
+		exit 1; \
+	fi


Why did you remove the passthrough target to mage?

rvolosatovs · 2019-04-29T08:27:54Z

.mage/rules.make

@@ -0,0 +1,257 @@
+# Copyright © 2019 The Things Network Foundation, The Things Industries B.V.


I thought we're migrating to Mage in this PR? Why do we need these?

rvolosatovs · 2019-04-29T08:28:36Z

Makefile

+	@echo "  run \"make build-all\""
+
+.PHONY: build-all
+build-all:


Why is this still here?

rvolosatovs · 2019-04-29T08:29:18Z

Makefile

@@ -12,30 +12,49 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.



Why do we still have targets here? Aren't we migrating to mage?

bafonins · 2019-04-29T09:18:38Z

in fresh repo

export NODE_ENV=production
make init deps
mage js:build

gives:

Error: Couldn't find preset "ttn" relative to directory "$GOPATH/src/go.thethings.network/lorawan-stack/sdk/js"
    at /usr/local/lib/node_modules/babel-cli/node_modules/babel-core/lib/transformation/file/options/option-manager.js:293:19
    at Array.map (<anonymous>)
    at OptionManager.resolvePresets (/usr/local/lib/node_modules/babel-cli/node_modules/babel-core/lib/transformation/file/options/option-manager.js:275:20)
    at OptionManager.mergePresets (/usr/local/lib/node_modules/babel-cli/node_modules/babel-core/lib/transformation/file/options/option-manager.js:264:10)
    at OptionManager.mergeOptions (/usr/local/lib/node_modules/babel-cli/node_modules/babel-core/lib/transformation/file/options/option-manager.js:249:14)
    at OptionManager.init (/usr/local/lib/node_modules/babel-cli/node_modules/babel-core/lib/transformation/file/options/option-manager.js:368:12)
    at File.initOptions (/usr/local/lib/node_modules/babel-cli/node_modules/babel-core/lib/transformation/file/index.js:212:65)
    at new File (/usr/local/lib/node_modules/babel-cli/node_modules/babel-core/lib/transformation/file/index.js:135:24)
    at Pipeline.transform (/usr/local/lib/node_modules/babel-cli/node_modules/babel-core/lib/transformation/pipeline.js:46:16)
    at transform (/usr/local/lib/node_modules/babel-cli/lib/babel/util.js:50:22)
error Command failed with exit code 1.                                                           
Error: running "node_modules/.bin/yarn --cwd=sdk/js run build" failed with exit code 1

johanstokking · 2019-04-30T09:57:36Z

.mage/translations.js

@@ -202,7 +202,7 @@ const readMessages = async function () {
 }

 /**
- * Read and parse (and marshal) the backend messages, coming from `make go.messages`
+ * Read and parse (and marshal) the backend messages, coming from `./mage go.messages`


htdvisser · 2019-04-30T10:58:13Z

@rvolosatovs we got rid of the proxy, now we run make init to set up Mage, after which we run ./mage <rules...>.

htdvisser · 2019-04-30T10:59:53Z

Notes to all:

.make/dev.make will be rewritten to Mage in a later PR
.mage/scripts/fix-grpc-gateway-names.sh will be rewritten to Mage in a later PR

htdvisser added the in progress We're working on it label Mar 28, 2019

htdvisser added this to the Next Up milestone Mar 28, 2019

htdvisser self-assigned this Mar 28, 2019

htdvisser requested a review from johanstokking March 28, 2019 09:56

htdvisser requested review from bafonins, kschiffer and rvolosatovs as code owners March 28, 2019 09:56

johanstokking previously approved these changes Mar 28, 2019

View reviewed changes

htdvisser force-pushed the feature/11-mage-dev-tooling branch from c0f6bb9 to bda911c Compare April 1, 2019 09:04

kschiffer mentioned this pull request Apr 2, 2019

Add script to regenerate SDK API definitions #431

Merged

htdvisser force-pushed the feature/11-mage-dev-tooling branch 8 times, most recently from 160e1ec to bae1616 Compare April 3, 2019 15:30

htdvisser requested a review from a team as a code owner April 3, 2019 15:50

johanstokking removed the request for review from a team April 3, 2019 17:49

kschiffer force-pushed the feature/11-mage-dev-tooling branch from 12ef488 to 29c7467 Compare April 4, 2019 12:19

bafonins reviewed Apr 5, 2019

View reviewed changes

kschiffer force-pushed the feature/11-mage-dev-tooling branch 2 times, most recently from 4a7b47f to f6c0e73 Compare April 5, 2019 08:31

kschiffer requested a review from bafonins April 5, 2019 11:22

htdvisser force-pushed the feature/11-mage-dev-tooling branch from f6c0e73 to 3204db7 Compare April 5, 2019 13:57

dev: Rewrite Golang Make targets to Mage

11385bf

kschiffer added 5 commits April 28, 2019 18:04

dev: Add target source file checks

09597f6

dev: Add necessity check to link target

f929f16

dev: Improve js target output

b7354c7

dev: Add js clean deps targets

fdb7892

dev: Remove unnecessary target dependency

1d6c019

kschiffer force-pushed the feature/11-mage-dev-tooling branch from 046524a to 1d6c019 Compare April 28, 2019 16:05

kschiffer approved these changes Apr 29, 2019

View reviewed changes

rvolosatovs suggested changes Apr 29, 2019

View reviewed changes

bafonins self-requested a review April 29, 2019 08:43

dev: Ensure protoc is available, do not update

82c670f

kschiffer added 3 commits April 29, 2019 16:23

dev: Fix sdk production builds

d27bfe3

dev: Fix webpack config on production builds

a7816aa

dev: Fix production build warning

234bf2b

bafonins approved these changes Apr 29, 2019

View reviewed changes

htdvisser added 4 commits April 30, 2019 11:42

dev: Move repo init to Mage and mage to root

68a29c6

ci: Fix release script

6b4aff0

doc: Update development documentation

30a5fe3

Merge branch 'ttn-master' into feature/11-mage-dev-tooling

c7d5e68

johanstokking reviewed Apr 30, 2019

View reviewed changes

htdvisser added 2 commits April 30, 2019 11:58

dev,doc: Fix typo

dbc4d37

dev: Fix dev:certificates target

61eac7d

htdvisser force-pushed the feature/11-mage-dev-tooling branch from 0585342 to 61eac7d Compare April 30, 2019 10:48

htdvisser requested a review from rvolosatovs April 30, 2019 10:56

htdvisser merged commit 9a7f7ca into master Apr 30, 2019

htdvisser deleted the feature/11-mage-dev-tooling branch April 30, 2019 11:59

htdvisser mentioned this pull request May 1, 2019

Fix release step of CI build #605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Development Tooling #377

Development Tooling #377

htdvisser commented Mar 28, 2019 •

edited

Loading

kschiffer commented Apr 4, 2019

bafonins left a comment

bafonins Apr 5, 2019

bafonins Apr 5, 2019

kschiffer Apr 5, 2019

bafonins Apr 5, 2019

kschiffer Apr 5, 2019

bafonins Apr 5, 2019

kschiffer commented Apr 5, 2019

rvolosatovs Apr 29, 2019

htdvisser Apr 29, 2019

johanstokking Apr 29, 2019

rvolosatovs Apr 29, 2019

rvolosatovs Apr 29, 2019

rvolosatovs Apr 29, 2019

rvolosatovs Apr 29, 2019

rvolosatovs Apr 29, 2019

rvolosatovs Apr 29, 2019

rvolosatovs Apr 29, 2019

bafonins commented Apr 29, 2019 •

edited

Loading

johanstokking Apr 30, 2019

htdvisser Apr 30, 2019

htdvisser commented Apr 30, 2019

htdvisser commented Apr 30, 2019

		@@ -0,0 +1,257 @@
		# Copyright © 2019 The Things Network Foundation, The Things Industries B.V.

		@@ -12,30 +12,49 @@
		# See the License for the specific language governing permissions and
		# limitations under the License.

Development Tooling #377

Development Tooling #377

Conversation

htdvisser commented Mar 28, 2019 • edited Loading

Release Notes

kschiffer commented Apr 4, 2019

bafonins left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschiffer commented Apr 5, 2019

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

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

bafonins commented Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htdvisser commented Apr 30, 2019

htdvisser commented Apr 30, 2019

htdvisser commented Mar 28, 2019 •

edited

Loading

bafonins commented Apr 29, 2019 •

edited

Loading