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

Deduplicate the assets directory #375

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

hannaeko
Copy link
Member

@hannaeko hannaeko commented Dec 13, 2022

Purpose

Since the migration to @angular/localize one bundle is generated per locale. This duplicates the assets.

While for the images it is sub-optimal but acceptable, for the configuration it is not. A user should not be required to update 6 times (or more in the future) the configuration, it is unpractical and error prone.

This PR changes the way the release zip is built to only use one assets folder. The asset folder is colocated with the locales folder to form the following architecture.

└── dist
    ├── assets
    │   ├── css
    │   ├── faqs
    │   ├── favicon
    │   └── images
    ├── da
    ├── en
    ├── es
    ├── fi
    ├── fr
    ├── nb
    └── sv

This way the assets folder is only present once. Also it means that our previous documentation still applies as the configuration path as not changed. Users will not have to move/copy the configuration file else where and will be able to reuse the existing one.

Context

#374

Changes

  • Use only one asset directory for all locales.
  • Correct a typo in documentation

How to test this PR

  • Deploy the GUI using the release zip and apache configuration.
  • Images (ZM logo) should be displayed and configuration should be accessible (try setting a message banner for instance).
  • FAQ should be accessible.

@hannaeko hannaeko added the T-Bug Type: Bug in software or error in test case description label Dec 13, 2022
@hannaeko hannaeko added this to the v2022.2 milestone Dec 13, 2022
@tgreenx tgreenx linked an issue Dec 13, 2022 that may be closed by this pull request
Copy link
Contributor

@tgreenx tgreenx left a comment

Choose a reason for hiding this comment

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

Works as expected, and also when using a custom base url.
Tested as part of 2022-2 release testing on Debian 11.

@matsduf
Copy link
Contributor

matsduf commented Dec 13, 2022

@blacksponge, I am not against it, but what risks do you see doing this very late in the release cycle?

@hannaeko
Copy link
Member Author

I am not against it, but what risks do you see doing this very late in the release cycle?

I did the smallest changes that I could to reduce the risk, and it has already been tested on two OSs. I do not forsee much risks coming from that but I still prefer to take it than deferring to next release and have something half finished for this one.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I think this is fine to include it within this release.

@hannaeko hannaeko merged commit 12b8316 into zonemaster:develop Dec 14, 2022
@matsduf matsduf added the V-Patch Versioning: The change gives an update of patch in version. label Dec 15, 2022
@matsduf
Copy link
Contributor

matsduf commented Dec 16, 2022

@blacksponge, is issue #374 fully resolved by this PR?

@hannaeko
Copy link
Member Author

is issue #374 fully resolved by this PR?

Yes it is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Bug Type: Bug in software or error in test case description V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update in Configuration.md and Installation.md needed
3 participants