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

Build system #61

Merged
merged 13 commits into from
Oct 27, 2015
Merged

Build system #61

merged 13 commits into from
Oct 27, 2015

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Oct 26, 2015

Build and publish tasks for sense as a plugin.

build: npm run build – clean, lint, copy, zip
release: npm run release – confirm, build, upload

This also includes basic linting with eslint. The Airbnb rules are used as they are planned for use in Kibana (probably once 4.3 is released). These rules are very comprehensive so many exceptions were needed to make the build pass. In order to cut down on noise (5361 errors) most rules have been disabled. Only the no-undef rule, which checks that a variable is defined before use, was left as a warning (28 warnings).

@spalger
Copy link
Contributor Author

spalger commented Oct 27, 2015

@bleskes this is ready for a look

eslint: {
source: {
src: [
'public/**/*.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering - no CSS/less linting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've never used one, maybe worth exploring later

@bleskes
Copy link
Contributor

bleskes commented Oct 27, 2015

@spalger Thx. I'll fix the warnings once we get this in. I did run into an issue trying to use this - the result tar ball has the original index.js file, which refers to the test app. The code for that doesn't ship, so the Kibana installer complains about missing code references.

@bleskes
Copy link
Contributor

bleskes commented Oct 27, 2015

we should probably also change the contriubting guide with instruction for the linting.

@spalger
Copy link
Contributor Author

spalger commented Oct 27, 2015

@bleskes ready for another look

];

if (existsSync(resolve(__dirname, 'public/tests'))) {
apps.push({
Copy link
Contributor

Choose a reason for hiding this comment

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

long live consts :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotta love em :) the new linting loves them too.

@spalger spalger mentioned this pull request Oct 27, 2015
@bleskes
Copy link
Contributor

bleskes commented Oct 27, 2015

Commented on the wrong issue. Copied over here:

it seems we fail to replace @SENSE_REVISION with the git sha
image

@bleskes
Copy link
Contributor

bleskes commented Oct 27, 2015

can release not build but check that there is an existing binary for the right version? I always want to check the binary before uploading. However, if I run release it destroys the binary I just checked and now I'm unsure..

@spalger
Copy link
Contributor Author

spalger commented Oct 27, 2015

I'm sure there is a way to test for the existence of the build, but that is what the confirmation is for. If you select no then it will simply build (or you can just run npm run build)

@spalger
Copy link
Contributor Author

spalger commented Oct 27, 2015

Fixing the replacement of @@SENSE_REVISION now

@bleskes
Copy link
Contributor

bleskes commented Oct 27, 2015

@spalger and I talked about it and we can let release rebake the tar ball for now. The reasoning is that now that we have a controlled latest url, we can safely upload broken s3 binaries without anyone downloading them by accident.

@bleskes
Copy link
Contributor

bleskes commented Oct 27, 2015

LGTM

@spalger spalger merged commit 4d28488 into elastic:master Oct 27, 2015
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.

2 participants