Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

TP: replace compass/ruby#6278

Merged
ocket8888 merged 12 commits intoapache:masterfrom
shamrickus:tp/remove-ruby
Oct 15, 2021
Merged

TP: replace compass/ruby#6278
ocket8888 merged 12 commits intoapache:masterfrom
shamrickus:tp/remove-ruby

Conversation

@shamrickus
Copy link
Copy Markdown
Member

@shamrickus shamrickus commented Oct 13, 2021

This PR changes the TP sass compiler from compass (deprecated) to dart-sass (official sass compiler). As a result, ruby is no longer a dependency of TP.

It also changes the TP build docker file to always install nodejs 12.


Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Portal
  • CDN in a Box

What is the best way to verify this PR?

Note: CiaB (and associated CI) for TP build will always fail (until merged) as it is using the dockerhub TP image, which was built using nodejs 10. gulp-dart-sass requires nodejs 12.

Build TP locally and confirm that it works.
Build TP using pkg with the -b command (so it uses the correct nodejs version).
Verify that TP looks correct.
Check the docs and make sure ruby/compass are no longer mentioned.

It is also possible to diff dist/resources/styles and see that the CSS is mostly the same with a few exceptions. First, sourceMaps are now specified by comments instead of through media queries. Second, the compass minification doesn't remove some chars that dart-sass does, as a result the new CSS should always be at least as small.

PR submission checklist

@ocket8888 ocket8888 self-assigned this Oct 13, 2021
Comment thread docs/source/development/building.rst Outdated
@zrhoffman
Copy link
Copy Markdown
Member

#6278's TP GHA failed due to #6283 needs to be rebased onto master now that #6284 is merged in order to pass

Comment thread docs/source/development/building.rst Outdated
Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

The grunt watch task still references "compass", which means that live-reloading (or what passes for it in AngularJS) is currently broken on this branch.

Copy link
Copy Markdown
Contributor

@ocket8888 ocket8888 left a comment

Choose a reason for hiding this comment

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

Changes look good, UI appears unchanged, RPMs build fine

As noted in the PR description, actions are expected to fail due to bad published images on Dockerhub. The changes in this PR include a fix for that, but they can't be updated until it's merged.

@ocket8888 ocket8888 merged commit 8ce81f5 into apache:master Oct 15, 2021
@zrhoffman
Copy link
Copy Markdown
Member

Note: CiaB (and associated CI) for TP build will always fail (until merged) as it is using the dockerhub TP image, which was built using nodejs 10. gulp-dart-sass requires nodejs 12.

As noted in the PR description, actions are expected to fail due to bad published images on Dockerhub. The changes in this PR include a fix for that, but they can't be updated until it's merged.

Our build-rpms action checks for whether the GO_VERSION file was changed in the last commit (or in the PR, if it's being run on a PR), and, if so, builds the builder image instead of pulling it:

if <<<"$files_changed" grep '^GO_VERSION$'; then
pkg_command+=(-b)
fi

So the action would pass if you added the TP builder Dockerfile to the list of files to check for, in addition to GO_VERSION (see #6210, which adds the TR builder Dockerfile).

zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Nov 5, 2021
* Replace compass with dart-sass

* Wrong sass

* Wrong sass

* More removals

* Changelog

* Cleanup

* Use exact versioning

* Remove other ruby deps

* Fix docs

* Fix docs

* Fix docs

* Fix watch

(cherry picked from commit 8ce81f5)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants