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

Extract Sage 9 compiler #1030

Closed
wants to merge 1 commit into from

Conversation

tangrufus
Copy link
Collaborator

@tangrufus tangrufus commented Nov 18, 2018

Second attempt of #997 plus compile multiple sage themes


What's wrong with #997?

Technically nothing's wrong. However, changing from project_local_path to project_build_path confuses developers (#997 (comment), #997 (comment)). Using a different variable name is not enough to show the differences between those variables:

  • project_local_path: the local bedrock path, e.g: /path/to/trellis/../site/example1, the one that you developing on
  • project_build_path: the temporary build path, under $TMPDIR/trellis or /tmp/trellis, not respecting project.repo_subtree_path

Thus, this attempt removes the needs of changing deploy-hooks/build-before.yml and requires develoeprs to add their sage theme names in group_vars/<env>/wordpress_sites.yml:

  # group_vars/<env>/wordpress_sites.yml
  wordpress_sites:
    example.com:
+     sage_themes:
+       - my-first-sage-theme-name
+       - my-second-sage-theme-name

Why multiple sage theme? Or, In what cases it will be helpful?

Testing needed!!

We have been using it on production for a while with file structure described here in #883 (comment)

Never tested on the roots-offical-docs way.

Discussion: Ansible Galaxy Role?

Since deploy-hooks/build-before.yml is useless when you don't use sage and support for Laravel projects is planned #951, would it make sense if we extract build-before.yml into a separate ansible galaxy role?


cc: @codepuncher, @BufferOverNo, @JackAlexander1

@swalkinshaw
Copy link
Member

Since deploy-hooks/build-before.yml is useless when you don't use sage and support for Laravel projects is planned #951, would it make sense if we extract build-before.yml into a separate ansible galaxy role?

Might a good idea.

As is right now, I don't like having sage_themes leaked into wordpress_sites.yml by default (even if it's just a comment).

I'm not sure how generic we can make this. Composer + Yarn install are common/generic. yarn build:production is more Sage specific, and then especially copying the dist folder.

@tangrufus
Copy link
Collaborator Author

tangrufus commented Nov 18, 2018

I'm not sure how generic we can make this. Composer + Yarn install are common/generic. yarn build:production is more Sage specific, and then especially copying the dist folder.

Instead of making compile steps (composer, yarn, copy, etc) generic, I suggest normalizing the way we complie frontend assets in deploy hooks; similar idea to #896

Reasons:

  • provide a standard way for thirdy party roles to extend trellis
  • don't bunlde unnecessary tasks in trellis
  • generic roles exist (e.g: geerlingguy.composer), no point in reinventing the wheel

For example, this is what developers would do if they using both sage 8 and sage 9 themes on the same server.

  # requirements.yml

+ - src: XXX.trellis-sage-8-compiler
+   version: 1.2.3

+ - src: YYY.trellis-sage-9-compiler
+   version: 1.2.3
  # group_vars/all/deploy-hooks.yml

+ deploy_build_before:
+   - "{{ playbook_dir }}/vendor/roles/XXX.trellis-sage-8-compiler/tasks/main.yml"
+   - "{{ playbook_dir }}/vendor/roles/YYY.trellis-sage-9-compiler/tasks/main.yml"
  # group_vars/<env>/wordpress_sites.yml

  wordpress_sites:
    example.com:
+     sage_8_compiler:
+       - my-sage-eight-theme
    another-example.com:
+     sage_9_compiler:
+       - my-sage-nine-theme
    multisite-example.com:
+     sage_9_compiler:
+       - my-sage-nine-theme
+       - my-second-sage-nine-theme
+     sage_8_compiler:
+       - my-sage-eight-theme
+       - my-second-sage-eight-theme

same idea goes to Laravel mix


As is right now, I don't like having sage_themes leaked into wordpress_sites.yml by default (even if it's just a comment).

How about this:

  • extract build-before.yml into a separate ansible galaxy role
  • remove deploy-hooks/build-before.yml from trellis
  • remove the comments from wordpress_sites.yml
  • galaxy role documents instruct developers to define sage_themes (or sage_8_compiler / sage_9_compiler) in wordpress_sites.yml

@swalkinshaw
Copy link
Member

Sounds like a good plan 👍

Only question is if it makes sense for the wp sites property sage_themes or whatever to be more generic? Maybe just theme_builder or something like that?

@tangrufus
Copy link
Collaborator Author

tangrufus commented Dec 16, 2018

Only question is if it makes sense for the wp sites property sage_themes or whatever to be more generic? Maybe just theme_builder or something like that?

I suggest not to make it generic to allow role authors to define whatever the structure(dict, array, string, boolean) they want. Only thing we expecting here is the property name should be unique.

This is because we don't want to limit role authors' flexibility. From the forum, I see people want to use Trellis on various frondend frameworks and static sites generator(e.g: Vue on top of Laravel, GatsbyJS, etc). They might require a very different kind of config in wordpress_sites.yml.


Let me know if you want go for extracting sage compiler into a separate ansible galaxy role.

@swalkinshaw
Copy link
Member

Yeah that makes sense. Go for it 👍

@tangrufus tangrufus changed the title build-before: Checkout project source code to local temporary directorry and compile multiple sage themes Extract Sage 9 compiler Jan 24, 2019
@tangrufus
Copy link
Collaborator Author

tangrufus commented Jan 24, 2019

Help wanted

Although I extracted to trellis-sage-9-compiler, there are some more tests and todos awaiting help.

See: https://github.com/ItinerisLtd/trellis-sage-9-compiler#todos-help-wanted


Question:

  • Should I completely remove sage from trellis, i.e: remove those comments from wordpress_sites.yml and remove the role from requirements.yml?

@swalkinshaw
Copy link
Member

Yeah I think we should remove all mentions of it, except for maybe an updated default deploy-hooks/build-before.yml?

Maybe deploy-hooks/build-before.yml could include a more generic simplified example with a npm install && npm build` type thing. It could also mention the sage compiler and link to docs?

Either way, we'd need to update our docs to include a page on Trellis + Sage I think. This would deserve more than an entry in the user contributed extensions page.

@retlehs what do you think?

@tangrufus
Copy link
Collaborator Author

we should remove all mentions of it,
deploy-hooks/build-before.yml could include a more generic simplified example with a npm install && npm build` type thing.

Done.

@swalkinshaw
Copy link
Member

@retlehs had some thoughts before on this. Can you explain them?

@retlehs
Copy link
Sponsor Member

retlehs commented Feb 13, 2019

i would prefer to close this PR entirely and just add the sage 9 compiler to the user contributed roles docs for if they want to use it...

we can't make it harder/more confusing to use sage out of the box with trellis

@swalkinshaw
Copy link
Member

Process now:

  • uncomment the example deploy hook

Process after:

  • add new galaxy role (+ install)
  • define deploy_build_before
  • update wordpress_sites

Is that correct? Obviously it's more work, but it's done in a more extensible way.

@retlehs
Copy link
Sponsor Member

retlehs commented Feb 13, 2019

if we're going to do this, there needs to be a theme agnostic ansible role that we can include in trellis by default

there shouldn't be anything specific to sage 8, sage 9, or even a theme - since plugins should be allowed to take advantage of this too

ideally there's some simple way to configure this in trellis (maybe more inline with how a CI config looks?), but i don't have a better idea of what that could be right now

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.

None yet

3 participants