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

Allow the site static content to be built on netlify #3015

Merged
merged 1 commit into from
Jan 3, 2019

Conversation

kevinsimper
Copy link
Contributor

This allows the site to be built on netlify without the precompiled static resources in the repo.

This also locks down which versions of the npm modules that the site depends on so everyone is sure they get the same deps that the site was tested with.

Also by not installing it globally, it makes it easier for new collaborators as they just have to do a npm install and not install something into their global modules folder.

@istio-testing
Copy link
Contributor

Hi @kevinsimper. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@geeknoid
Copy link
Contributor

geeknoid commented Jan 2, 2019

I'm not sure of the value here. In the current state, if you use the prescribed approach to interact with the site content, everyone gets consistent builds without needing to deal with npm at all. "make serve", "make build", and "make gen" all use a consistent docker image that ensures command-line, circleci, and netlify use are all identical.

Having Netlify build the static content means that it's no longer using the bits that are checked in, and so things can get out of sync between what people do on the command-line and what circleci is exposed to.

@kevinsimper
Copy link
Contributor Author

if you use the prescribed approach to interact with the site content, everyone gets consistent builds without needing to deal with npm at all.

Yes, absolute, that is a good idea! Everyone would still be able to use that approach after this PR, it only makes other things possible as well :)

"make serve", "make build", and "make gen" all use a consistent docker image that ensures command-line, circleci, and netlify use are all identical.

This doesn't change it, it just puts the dependencies into package.json for best practice and allows other things.

Having Netlify build the static content means that it's no longer using the bits that are checked in, and so things can get out of sync between what people do on the command-line and what circleci is exposed to.

We can disable that and people can continue to check in the compiled assets :)


There is a couple of benefits of having the dependencies in package.json. Watching files with Docker for Mac is often really difficult as file events does not work that well compared to native, so if you want to implement a small neat feature like a watcher, it would be easier to make a small make watch-css with a npm package that watches sass :)

If Netlify can also build the PR's, it would also make it possible to the CSS and JS changes in the website that Netlify deploys for each PR, making it super easy to verify small changes :)

Putting the dependencies into package.json also makes it easier to see what version of a package is installed when looking for the docs, if you wanted to see which version of sass the website was using you would have to look into the docker image and run it and look inside.

I don't want to force anything and I want to help contribute to the istio website. I am a front-end developer and think it is a awesome project that you guys have built! There is so much value coming from this and much more from the future!

@geeknoid
Copy link
Contributor

geeknoid commented Jan 2, 2019

I am not a front-end developer, and I've put together all the istio.io infrastructure such as it is. So I appreciate any help and insights :-)

Ultimately, I want to switch to use Hugo pipes, which will eliminate the need to run a build step for CSS and JS. Alas, this is blocked on this. I had tried to make this work in this PR, but Netlify isn't ready yet :-(

@geeknoid
Copy link
Contributor

geeknoid commented Jan 2, 2019

So how about if you just undid the change to netlify.toml? The rest looks fine, I just don't want Netlify to be doing something different than what happens on the command-line and on CircleCi.

Thanks.

@kevinsimper
Copy link
Contributor Author

I am not a front-end developer, and I've put together all the istio.io infrastructure such as it is. So I appreciate any help and insights :-)

I think it is a good job and I didn't want to come in and make changes to everything so I appreciate the welcomeness 👍 😄

Ultimately, I want to switch to use Hugo pipes, which will eliminate the need to run a build step for CSS and JS. Alas, this is blocked on this. I had tried to make this work in this PR, but Netlify isn't ready yet :-(

That would be pretty cool, seems really simple! 👍

So how about if you just undid the change to netlify.toml? The rest looks fine, I just don't want Netlify to be doing something different than what happens on the command-line and on CircleCi.

I updated the PR with your suggestion 😄

@geeknoid
Copy link
Contributor

geeknoid commented Jan 3, 2019

/approve
/lgtm

@istio-testing
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid, kevinsimper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@istio-testing istio-testing merged commit e5e8875 into istio:master Jan 3, 2019
@kevinsimper
Copy link
Contributor Author

@geeknoid Thank you for the help 😄

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.

4 participants