blue-build / modules

BlueBuild standard modules used for building your Atomic Images
Apache License 2.0
22 stars 26 forks source link

docs: new directory-structure standard and other guidelines #157

Closed fiftydinar closed 2 months ago

fiftydinar commented 5 months ago

Intro

Instead of using /usr/share/ublue-os & /usr/etc/ublue-os or other non-standard directories for modules, we will use new BlueBuild module directories for module configs:

Purpose

  1. /usr/share/bluebuild/module-name is utilized strictly for maintainer config, which can't be modified by users.
  2. /etc/bluebuild/module-name is utilized for user config on booted system.
  3. /usr/etc/bluebuild/module-name is utilized for resetting user config easily, by copying files from that directory to /etc/bluebuild/module-name

For this structure to work successfully, implementation of some mechanisms is required in the module, depending on the module type.

By mechanisms, I mean ensuring that user config is separated cleanly from the maintainer config, which is not as easy as simply using /usr/etc location & being happy.

User config won't look the same as maintainer config, which is on purpose, to make that separation clearer. This would make the module troubleshooting easier, without need of the maintainer to manually check the differences of what the user modified from original maintainer config.

Advantages

For example, if maintainer used this config for default-flatpaks module with flatpak installation list in /usr/etc/bluebuild/default-flatpaks/system/install config:

org.gnome.Boxes
org.gnome.Calculator
org.gnome.Calendar
org.gnome.Snapshot
org.gnome.Contacts
org.gnome.clocks
org.gnome.Evince
org.gnome.Maps
org.gnome.TextEditor
com.github.neithern.g4music
com.github.rafostar.Clapper
org.gnome.Loupe
org.gnome.Epiphany
io.missioncenter.MissionCenter
com.vixalien.sticky
com.github.flxzt.rnote
org.localsend.localsend_app

But user only wants to include this: org.gnome.World.Secrets

He wouldn't need to edit flatpak list to be like this in /etc/bluebuild/default-flatpaks/system/install config (hard to spot the difference):

org.gnome.Boxes
org.gnome.Calculator
org.gnome.Calendar
org.gnome.Snapshot
org.gnome.Contacts
org.gnome.clocks
org.gnome.Evince
org.gnome.Maps
org.gnome.TextEditor
com.github.neithern.g4music
com.github.rafostar.Clapper
org.gnome.World.Secrets
org.gnome.Loupe
org.gnome.Epiphany
io.missioncenter.MissionCenter
com.vixalien.sticky
com.github.flxzt.rnote
org.localsend.localsend_app

But he can just enter this value that he wants like this with this new folder structure:

org.gnome.World.Secrets

Why? Because we can implement the dynamic logic of when maintainer & user config can be considered & when not.

One example of this logic is to use:

etc.

I implemented the logic similar to the example above with this PR: https://github.com/ublue-os/bling/pull/122/

We would document use of useful commands like this, which would help module-maintainers in saving-time when making or maintaining a module.

Another advantage is that we can document module files clearly with this separation, while with previous config we could not. F.e. we can document:

"This file is used by the system for maintainer-config, don't modify it" "This file can be used by the user"

while with previous config, you're stuck with 1 version of documentation only.

xynydev commented 5 months ago

What's the motivation for having some module configuration be user-editable post-boot? I mean, it seems kind of odd to me, but with the default-flatpaks issue it seems to make some sense too. I'm just not sure if the feature is enough to warrant added complexity in all modules that it's used in.

Also, the separation between and best practices around /usr/share/bluebuild/ and /usr/etc/bluebuild/ should be more clearly written out when these guidelines are written. I'm not sure whether a systemd unit, for example, should be copied straight into place, or symlinked from /usr/share/bluebuild/. If not, what's the type of file that would go into /usr/share/bluebuild/? Is default-flatpaks currently the only module with a need for such a separation?

fiftydinar commented 5 months ago

What's the motivation for having some module configuration be user-editable post-boot? I mean, it seems kind of odd to me, but with the default-flatpaks issue it seems to make some sense too. I'm just not sure if the feature is enough to warrant added complexity in all modules that it's used in.

This is a good question. User-editable config should be used only for modules which:

Also, the separation between and best practices around /usr/share/bluebuild/ and /usr/etc/bluebuild/ should be more clearly written out when these guidelines are written. I'm not sure whether a systemd unit, for example, should be copied straight into place, or symlinked from /usr/share/bluebuild/. If not, what's the type of file that would go into /usr/share/bluebuild/?

I agree.

Those simple configuration files should contain documentation about:

Documentation should always start with # symbol, to exclude it as code.

The type of config file you make is a simple text file without any extension, with name of the config you implement (f.e. "notifications" file name for configuring notifications, "install" for installing flatpak, "remove" for removing flatpak). For better file organization, you can also group those configs in folders if needed.

#

If you find it too complicated to achieve those 2 config locations above due to module complexity, you can ask other people for help, or you can resort to this structure only if you have to & if BlueBuild reviewers approve as passable:

In this case, you document everything based on guidelines explained above, with little changes:

There is also no need to implement code mechanisms for detecting maintainer & user config in this case, as user's config automatically mirrors maintainer's config as a starting template, which he can modify. You just need to be sure that you are using /etc/bluebuild/module-name location for grabbing the config values.

However, this has cons that user would need to update his config constantly if maintainer changes his config often, as changes in /etc are not caught from /usr/etc when user changes the file (they are detected as edited & are excluded from /usr/etc updates by ostree I think).

Everything else is same as above.

Benefits I outlined in 1st post are lost if this structure is used.

Code examples can also be included to make the guidelines following easier.

Is default-flatpaks currently the only module with a need for such a separation?

Yes.

Other modules don't fit the outlined principles which would require us to implement configuration options.

If we talk about unofficial modules too, my initramfs-setup module fits this usecase, as it offers option for users to include their own initramfs arguments, which can be very useful for specific hardware.

xynydev commented 5 months ago

Thanks for the in-depth comment. I think I'm coming to the realization that the guidelines for official modules should be in the modules repositories README. This is a clear separation; user documentation and general contributing guide on the website, repository-specific developer documentation inside each repository. We can keep this discussion here for now, then transfer it once the repo is moved.

fiftydinar commented 5 months ago

Raised an issue relevant to this topic above, I recommend to check that out.

xynydev commented 4 months ago

I have removed the guidelines for official modules from the website completely, as these are official BlueBuild module -specific guidelines and should be in the module's repo's README.

Here's what's removed, to be added to the modules repo later:

Style guidelines for official modules

These are general guidelines for writing official modules and their documentation to follow to keep a consistent style. Not all of these are to be mindlessly followed, especially the ones about grammar and writing style, but it's good to keep these in mind if you intend to contribute back upstream, so that your module doesn't feel out of place.

Bash

module.yml (not implemented) (will replace READMEs)

Each public module should also include a module.yml with the following keys: (TODO, not planned yet).

description:

xynydev commented 4 months ago

Transferred to the modules repo. Contributions appreciated.

Regarding this topic more broadly, we currently need:

xynydev commented 4 months ago

One topic I'd like to discuss is the name format of variables and functions. My intuition would say: VARIABLE_NAME & function_name, but lowercase variable names could also be used.

fiftydinar commented 4 months ago

One topic I'd like to discuss is the name format of variables and functions. My intuition would say: VARIABLE_NAME & function_name, but lowercase variable names could also be used.

"VARIABLE_NAME" is better for reading, but "variable_name" is better for separating existing environment variables from script variables.

So I would opt for "ENVIRONMENT_VARIABLE", "script_variable" & "function_name".

fiftydinar commented 4 months ago
  1. I learnt this when I was reading Universal Blue's script. Use {}, to better enclose string variables, like "${variable_name}". That is a good practice, which we should respect.

  2. Always use quotes whenever possible for strings, to ensure that they are parsed correctly.

    There is an exception that individual arrays [@] can get processed as a whole array [*] when using quotes in a single string, but solution to that is to use for/done loop for those [@], as that is a more reliable method anyway. But if you must use single [@] string, then you should not use quotes for them, as I had this exact same issue with 1st commits here: https://github.com/blue-build/modules/pull/83

    Example: Flatpak would complain that it can't install flatpaks with more than 255 characters when I used "${INSTALL_LIST[@]}" instead of ${INSTALL_LIST[@]}, which separated array elements correctly.

    Another exception is wildcard expansion using or similar, which should not be quoted, as it will be treated as literal string. F.e. instead of copying every file from my_dir, you would copy literal file named `inmy_dir`.

  3. We should make modules as portable as possible, reviewing current dependencies for modules & seeing if we can use some more portable dependencies (using gnu coreutils programs instead of some downstream ones as an example & similar)

bayou-brogrammer commented 4 months ago

okay I finally read through this and ill give my input.

Bash

  • Echo what you're doing on each step and on errors to help debugging.
  • Don't echo blocks using "===", that is reserved for the code launching the modules.
  • Use snake_case for functions and variables changed by the code.
  • Use SCREAMING_SNAKE_CASE for variables that are set once and stay unchanged.

module.yml (not implemented) (will replace READMEs)

Each public module should also include a module.yml with the following keys: (TODO, not planned yet).

description:

  • At the start of each paragraph, refer to the module using its name or with "the module", not "it" or "the script"
  • Use passive grammar when talking about the user, ie. "should be used", "can be configured", preferring references to what the module does, ie. "This module downloads the answer to the question of life, the universe and everything..."

I agree with this stance. env vars and const should be SCREAMING_SNAKE_CASE. i have no preference on the others.

like @fiftydinar states variables should be placed like "${variable_name}" to prevent globbing.

/usr/etc/bluebuild/module-name Maintainer's configuration

/etc/bluebuild/module-name User configuration

This makes sense to me. core modules are placed in /etc while user overrides are pulled from /usr.

We want to avoid a situation where we have users completely removing our module because it was placed in the same location. which is what is happening on bluefin atm

fiftydinar commented 4 months ago

/usr/etc/bluebuild/module-name Maintainer's configuration /etc/bluebuild/module-name User configuration

This makes sense to me. core modules are placed in /etc while user overrides are pulled from /usr.

I think there is a misunderstanding in this part.

We want to use:

System configuration: /usr/share/bluebuild/module-name

Local configuration: /usr/etc/bluebuild/module-name (which is then copied to /etc)

I see that "System" & "Local" term is better for usage than previous "Maintainer" & "User". xynydev helped me with this.

The reason why we use /usr/share location instead of /usr/etc for system configuration is because we want to avoid rpm-ostree handling between /etc & /usr/etc modifications, which marks modified /etc configuration as non-updateable, which causes issues with local users not getting system updates to the module's system configuration file.

/usr/etc in this case is used just as a document template, which tells the user how to modify the file & what it does with 0 module entries, while /etc file contains local module entries only if local user inputted those, which can be easily reverted by just copying system file from /usr/etc without any issues.

If you want to understand things clearer on this regard, you can check how default-flatpaks module handles this.

We want to avoid a situation where we have users completely removing our module because it was placed in the same location. which is what is happening on bluefin atm

I think you refer to the scenario I described above with rpm-ostree /usr/etc & /etc handling? If so, this is solved with new structure as described above.

xynydev commented 4 months ago

I agree with this stance. env vars and const should be SCREAMING_SNAKE_CASE. i have no preference on the others.

(from @bayou-brogrammer, emphasis applied by me)

This was my original intention in the guidelines too, that constant variables (that are set once and stay unchanged) should be SCREAMING_SNAKE_CASE. I'd like to hear @fiftydinar's stance about the constant variables. These can't be considered env variables, because not all of these are meant to be consumed by other scripts.

fiftydinar commented 4 months ago

This was my original intention in the guidelines too, that constant variables (that are set once and stay unchanged) should be SCREAMING_SNAKE_CASE. I'd like to hear @fiftydinar's stance about the constant variables. These can't be considered env variables, because not all of these are meant to be consumed by other scripts.

I agree with this.

fiftydinar commented 3 months ago

The type of config file you make is a simple text file without any extension, with name of the config you implement (f.e. "notifications" file name for configuring notifications, "install" for installing flatpak, "remove" for removing flatpak). For better file organization, you can also group those configs in folders if needed.

I am going to quote myself here that I think that it's actually better to use some standardized config format like .yml instead of using plain text file & parsing that output with grep, awk or something else in a hacky way.

We ship yq to the images, so we should take the advantage of it.