1033415 - Remove the output of grunt build from the treeherder repos & generate on deployment

Status: RESOLVED WONTFIX

(Tree Management :: Treeherder, defect, P3)

Description

[Ed Morley [:emorley]]

Broken out from bug 1032495:

(In reply to Jonathan French (:jfrench) from comment #2)
> I've tested the fix and it appears to be fine. I have the change ready to
> push and open a PR, but I am getting somewhat crazy results when I grunt
> build . All sorts of unrelated changes in ./tmp/concat and ./dist. I need to
> check in with mdoglio or camd tomorrow on that before I open the PR. 

For me (as someone not that familiar with the treeherder codebase/repo layout; but who will likely need to/be asked to fix things in the future), it really harms the readability of commits when they include the output of grunt build (files under .tmp/ and .dist/). Worse, the output of grunt build hasn't already been checked in straight away - leaving confusing large commits made the next time someone does run it (eg https://github.com/mozilla/treeherder-ui/pull/88/files).

I think we should remove the generated files from the tree, add entries for them to the .gitignore, and adjust the production deploy scripts (and readthedocs dev install how-tos) to run grunt build after checking out the updated repo. 

Failing that, can we at least have a Travis test to ensure that every commit is made with it's corresponding generated grunt files, so they don't get out of sync?

Comment 1

[Ed Morley [:emorley]]

s/already/always/

Comment 2

[Jonathan French (:jfrench)]

+1. Thanks for entering this, I wasn't sure if it already existed or not.

Comment 3

[Jonathan Eads ( :jeads )]

It was not a development decision to include the 'dist/' grunt build output directly in the treeherder-ui repository. This was the only way we were able to get the grunt build output to production. There were numerous puppet related issues with adding the npm requirements to run the treeherder grunt build on the admin node in the production environment under chief. Until those IT related issues are resolved this is the easiest way we have to push out the results of a grunt build.

Until those issues are resolved our best strategy is to never include the grunt build results in github PR's. Incorporation of new grunt build results will only be done on an as needed basis when we push new code to stage/prod and will be done directly on master. Our development environment serves raw js/css for debugging purposes so we can test PR's there before pushing to stage/production without grunt build sync issues. 

The only long term solution here is to sort out the puppet npm issues with webops/IT.

Comment 4

[Ed Morley [:emorley]]

(In reply to Jonathan Eads ( :jeads ) from comment #3)
> Until those issues are resolved our best strategy is to never include the
> grunt build results in github PR's. Incorporation of new grunt build results
> will only be done on an as needed basis when we push new code to stage/prod
> and will be done directly on master. Our development environment serves raw
> js/css for debugging purposes so we can test PR's there before pushing to
> stage/production without grunt build sync issues. 

That sounds good to me - and removes the complexity of larger commits and/or commits including changes from other landings - thank you.

We'll need to remind new contributors that they should ignore grep results that they hit in dist/ but that's a much smaller issue overall.

Comment 5

[Ed Morley [:emorley]]

(In reply to Jonathan Eads ( :jeads ) from comment #3)
> There were numerous puppet
> related issues with adding the npm requirements to run the treeherder grunt
> build on the admin node in the production environment under chief. Until
> those IT related issues are resolved this is the easiest way we have to push
> out the results of a grunt build.

Are there bugs filed for those?

Comment 6

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #5)
> (In reply to Jonathan Eads ( :jeads ) from comment #3)
> > There were numerous puppet
> > related issues with adding the npm requirements to run the treeherder grunt
> > build on the admin node in the production environment under chief. Until
> > those IT related issues are resolved this is the easiest way we have to push
> > out the results of a grunt build.
> 
> Are there bugs filed for those?

Comment 7

[Jonathan Eads ( :jeads )]

Those bugs are not filed and until we do some research it doesn't make sense to file them. We should include explicit instructions for how we want to manage npm usage in production and I don't know what the best way forward there is at this point. Lets consider that research part of closing this bug.

Comment 8

[Ed Morley [:emorley]]

Given these files are now concatenated & minified, it's harder to confuse them for the real files, when grepping the repo. Given this and the difficulties with getting this running on the admin node (and the issues with not being able to verify the output created as part of deploy - I think we should wontfix (at least for now).