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 app to be used as a cockpit package #56

Merged
merged 2 commits into from Mar 29, 2017

Conversation

Projects
None yet
4 participants
@petervo
Contributor

petervo commented Mar 20, 2017

This really more of a proposal suggesting we run this web interface as a cockpit package, instead of requiring a stand alone server.

I'm sure this doesn't quite port everything over correctly and there are still issues to be worked out, but it's more of a POC of how this could happen.

Some of the basic principles are:

  1. All http requests made from the javascript need to use relative paths.
  2. There should be no inline js or css
  3. All requests to the api should be made using cockpit.http type requests, and not directly from the javascript.
  4. Because the app itself is loaded in a iframe, use hashes for navigation within the SPA. That way the cockpit shell can keep the top level navigation up to date.
@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Mar 21, 2017

Contributor

@petervo Most of the change is just plain cleanup. Should we move the code cleanup parts (ie: Using bower, and cleanup sitemap) into their own pull request?

Lets keep in mind that Weldr would want to be able to run both hosted mode as a general purpose website and used standalone on a Linux system in a Cockpit package. So:

  1. All http requests made from the javascript need to use relative paths.

This holds.

  1. There should be no inline js or css

Agree that having inline javascript or CSS is a bad security practice. But in the worst case, Cockpit can relax its strict default security if a component like Weldr is included. See the content-security-policy setting here: http://cockpit-project.org/guide/latest/packages.html#package-manifest

All requests to the api should be made using cockpit.http type requests, and not directly from the javascript.

Here's where the POC needs some adjustment. This needs to be done opportunistically, and only if running within Cockpit. I imagine there's a simple way to pull this off.

Because the app itself is loaded in a iframe, use hashes for navigation within the SPA. That way the cockpit shell can keep the top level navigation up to date.

This is just best practice anyway.

Contributor

stefwalter commented Mar 21, 2017

@petervo Most of the change is just plain cleanup. Should we move the code cleanup parts (ie: Using bower, and cleanup sitemap) into their own pull request?

Lets keep in mind that Weldr would want to be able to run both hosted mode as a general purpose website and used standalone on a Linux system in a Cockpit package. So:

  1. All http requests made from the javascript need to use relative paths.

This holds.

  1. There should be no inline js or css

Agree that having inline javascript or CSS is a bad security practice. But in the worst case, Cockpit can relax its strict default security if a component like Weldr is included. See the content-security-policy setting here: http://cockpit-project.org/guide/latest/packages.html#package-manifest

All requests to the api should be made using cockpit.http type requests, and not directly from the javascript.

Here's where the POC needs some adjustment. This needs to be done opportunistically, and only if running within Cockpit. I imagine there's a simple way to pull this off.

Because the app itself is loaded in a iframe, use hashes for navigation within the SPA. That way the cockpit shell can keep the top level navigation up to date.

This is just best practice anyway.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Mar 21, 2017

Contributor

Here's where the POC needs some adjustment. This needs to be done opportunistically, and only if running within Cockpit. I imagine there's a simple way to pull this off.

Didn't realize we wanted to keep old behavior as well. Should be relatively simple. I'll adjust tomorrow.

Contributor

petervo commented Mar 21, 2017

Here's where the POC needs some adjustment. This needs to be done opportunistically, and only if running within Cockpit. I imagine there's a simple way to pull this off.

Didn't realize we wanted to keep old behavior as well. Should be relatively simple. I'll adjust tomorrow.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Mar 22, 2017

Contributor

Updated so now it works both with and without cockpit.

Contributor

petervo commented Mar 22, 2017

Updated so now it works both with and without cockpit.

Show outdated Hide outdated core/constants.js
get_module_info: "/api/v0/projects/info/",
get_dependencies_list: "/api/v0/modules/info/",
get_compose_types: "/api/v0/compose/types",
post_recipes_new: "/api/v0/recipes/new",

This comment has been minimized.

@bcl

bcl Mar 23, 2017

Collaborator

This will need to rebase on the new master -- API constants should match the route path to keep things clear.

@bcl

bcl Mar 23, 2017

Collaborator

This will need to rebase on the new master -- API constants should match the route path to keep things clear.

@bcl

This comment has been minimized.

Show comment
Hide comment
@bcl

bcl Mar 23, 2017

Collaborator

This looks pretty good, thanks! How would I go about running this inside cockpit, from a f25 install of it?

Collaborator

bcl commented Mar 23, 2017

This looks pretty good, thanks! How would I go about running this inside cockpit, from a f25 install of it?

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Mar 23, 2017

Contributor

Simplest way build the project

node run.js build

Link the built files to cockpit's package dir.

mkdir -p ~/.local/share/cockpit
ln -s /path/to/welder-web/public ~/.local/share/cockpit/welder-web
Contributor

petervo commented Mar 23, 2017

Simplest way build the project

node run.js build

Link the built files to cockpit's package dir.

mkdir -p ~/.local/share/cockpit
ln -s /path/to/welder-web/public ~/.local/share/cockpit/welder-web
@bcl

This comment has been minimized.

Show comment
Hide comment
@bcl

bcl Mar 23, 2017

Collaborator

Thanks, so I've been able to test non-cockpit running from inside docker. I can't seem to get it to run from cockpit, it expects things like patternfly.min.css, etc. to be in .../welder-web/dist/public/

Collaborator

bcl commented Mar 23, 2017

Thanks, so I've been able to test non-cockpit running from inside docker. I can't seem to get it to run from cockpit, it expects things like patternfly.min.css, etc. to be in .../welder-web/dist/public/

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Mar 23, 2017

Contributor

Did you run bower install?

Contributor

petervo commented Mar 23, 2017

Did you run bower install?

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Mar 23, 2017

Contributor

If that is the problem I'll add that step to the build task in run.js

Contributor

petervo commented Mar 23, 2017

If that is the problem I'll add that step to the build task in run.js

@bcl

This comment has been minimized.

Show comment
Hide comment
@bcl

bcl Mar 24, 2017

Collaborator

Did you run bower install?

No, I didn't see it mentioned anywhere. Adding that to run.js would be a good idea since I'm not sure how to install it (tried npm install bower and it did some things, but isn't available anywhere that I can find on the PATH).

Collaborator

bcl commented Mar 24, 2017

Did you run bower install?

No, I didn't see it mentioned anywhere. Adding that to run.js would be a good idea since I'm not sure how to install it (tried npm install bower and it did some things, but isn't available anywhere that I can find on the PATH).

@jgiardino

This comment has been minimized.

Show comment
Hide comment
@jgiardino

jgiardino Mar 24, 2017

Contributor

I'm not an expert on this, but wanted to pass this feedback along from Patrick Riley regarding the use of bower vs npm:

So the only issue I see with that PR is the adoption of Bower. Almost all front end packages now are moving to NPM. Bower is kind of dying at this point (even though it's still technically viable).

If there's an issue with long-term support of bower, should we be using npm instead?

Contributor

jgiardino commented Mar 24, 2017

I'm not an expert on this, but wanted to pass this feedback along from Patrick Riley regarding the use of bower vs npm:

So the only issue I see with that PR is the adoption of Bower. Almost all front end packages now are moving to NPM. Bower is kind of dying at this point (even though it's still technically viable).

If there's an issue with long-term support of bower, should we be using npm instead?

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Mar 24, 2017

Contributor

If there's an issue with long-term support of bower, should we be using npm instead?

Yup. Especially for a newer project like this. Makes sense to think ahead.

Contributor

stefwalter commented Mar 24, 2017

If there's an issue with long-term support of bower, should we be using npm instead?

Yup. Especially for a newer project like this. Makes sense to think ahead.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Mar 25, 2017

Contributor

Ok I rebased and removed all the bower bits. Though I still think loading from external CDNs is not a good idea. I'll leave it alone for now.

Contributor

petervo commented Mar 25, 2017

Ok I rebased and removed all the bower bits. Though I still think loading from external CDNs is not a good idea. I'll leave it alone for now.

@jgiardino

This comment has been minimized.

Show comment
Hide comment
@jgiardino

jgiardino Mar 28, 2017

Contributor

I agree with you @petervo. It would be good to remove the CDN links and use npm for patternfly css instead.

Contributor

jgiardino commented Mar 28, 2017

I agree with you @petervo. It would be good to remove the CDN links and use npm for patternfly css instead.

@bcl

This comment has been minimized.

Show comment
Hide comment
@bcl

bcl Mar 29, 2017

Collaborator

Ok, I think this is working. It was a bit hard to figure out, so here's what ended up working:

  • git clean -xdf && npm install && node run build
  • ln -s /path/to/public /usr/share/cockpit/welder-web/

Restart cockpit after making changes and running node run build, otherwise the file hashes won't match what it expects and main.js won't load. That tripped me up for a bit.

@jgiardino I also agree that CDN sources shouldn't be used. We can change that in a separate PR though.

Collaborator

bcl commented Mar 29, 2017

Ok, I think this is working. It was a bit hard to figure out, so here's what ended up working:

  • git clean -xdf && npm install && node run build
  • ln -s /path/to/public /usr/share/cockpit/welder-web/

Restart cockpit after making changes and running node run build, otherwise the file hashes won't match what it expects and main.js won't load. That tripped me up for a bit.

@jgiardino I also agree that CDN sources shouldn't be used. We can change that in a separate PR though.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Mar 29, 2017

Contributor

@bcl you should link it to ~/.local/share/cockpit/welder-web instead. That way you only need to log out of cockpit when you first make the link. After that the hashes don't matter and a simple refresh will pick up changes after each rebuild. Of course you need to make sure you log into cockpit as the same user who's home dir you used.

Contributor

petervo commented Mar 29, 2017

@bcl you should link it to ~/.local/share/cockpit/welder-web instead. That way you only need to log out of cockpit when you first make the link. After that the hashes don't matter and a simple refresh will pick up changes after each rebuild. Of course you need to make sure you log into cockpit as the same user who's home dir you used.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Mar 29, 2017

Contributor

ln -s /path/to/public /usr/share/cockpit/welder-web/

During development do the following, and then hashes will be disabled, and you'll just be able to refresh Cockpit without logging out and logging back in:

  • ln -s /path/to/public ~/.local/share/cockpit/welder-web

Now log in with your local user. The one that owns ~

Contributor

stefwalter commented Mar 29, 2017

ln -s /path/to/public /usr/share/cockpit/welder-web/

During development do the following, and then hashes will be disabled, and you'll just be able to refresh Cockpit without logging out and logging back in:

  • ln -s /path/to/public ~/.local/share/cockpit/welder-web

Now log in with your local user. The one that owns ~

@bcl

This comment has been minimized.

Show comment
Hide comment
@bcl

bcl Mar 29, 2017

Collaborator

Thanks, I had tried that initially and it didn't work, but that was before removing bower so that's probably what the real issue was.

I think this is almost ready to merge. The commit message needs to be cleaned up (No WIP: commits allowed) and maybe add any guidelines we need to follow so that future development doesn't break cockpit compatibility. "Always use relative URLs" seems like the big one :)

Collaborator

bcl commented Mar 29, 2017

Thanks, I had tried that initially and it didn't work, but that was before removing bower so that's probably what the real issue was.

I think this is almost ready to merge. The commit message needs to be cleaned up (No WIP: commits allowed) and maybe add any guidelines we need to follow so that future development doesn't break cockpit compatibility. "Always use relative URLs" seems like the big one :)

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Mar 29, 2017

Contributor
Contributor

petervo commented Mar 29, 2017

@bcl

This comment has been minimized.

Show comment
Hide comment
@bcl

bcl Mar 29, 2017

Collaborator

I can do this, where should I put those instructions, commit message?
README?

A README would be best, so it doesn't get lost in the mists of time.

Collaborator

bcl commented Mar 29, 2017

I can do this, where should I put those instructions, commit message?
README?

A README would be best, so it doesn't get lost in the mists of time.

@petervo petervo changed the title from WIP: Run as a cockpit package to Allow app to be used as a cockpit package Mar 29, 2017

Allow app to be used as a cockpit package
Adds a cockpit package manifest and makes the following
changes to allow this app to be used as a cockpit package.

 * All urls in the html and javascript need to use relative paths
 * All requests to the API should be made
   using utils.apiFetch. Any non API fetch requests
   must use credentials: 'same-origin' so that
   cookies are included with those ajax requests.
 * Use hashes for navigation within the SPA
   so that cockpit can keep the top level location display
   up to date.
@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Mar 29, 2017

Contributor

Update readme and commit message

Contributor

petervo commented Mar 29, 2017

Update readme and commit message

@bcl

bcl approved these changes Mar 29, 2017

Looks good to me, thanks! @jgiardino what do you think?

@jgiardino jgiardino merged commit 0fb7210 into weldr:master Mar 29, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment