Skip to content

Break installation defaults out as their own structure. #83

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

Closed
wants to merge 1 commit into from

Conversation

albu-diku
Copy link
Contributor

@albu-diku albu-diku commented Jul 15, 2024

Add tests that assert the consistency of options accepted by the
generateconfs command line against the internal library function.
Additionally assert that the library routine itself matches the defaults
structure thus making the structure the definitive source of truth.

Doing so highlighted the following missing command line options which
are added as of this commit:
--seafile_secret
--seafile_ccnetid

@jonasbardino jonasbardino added the enhancement New feature or request label Jul 15, 2024
@albu-diku albu-diku force-pushed the refactor/split-up-config-generation branch from 7f6f448 to 9400618 Compare July 15, 2024 13:59
@albu-diku albu-diku force-pushed the refactor/split-out-install-defaults branch from 23ae04a to 3388101 Compare July 15, 2024 14:00
@albu-diku albu-diku force-pushed the refactor/split-out-install-defaults branch 2 times, most recently from 6b7936f to 24e8183 Compare July 22, 2024 09:26
@albu-diku albu-diku changed the title Break the installation defaults out into their own file. Break the installation defaults out as their own structure Jul 22, 2024
@albu-diku albu-diku changed the title Break the installation defaults out as their own structure Break the installation defaults out as their own structure. Jul 22, 2024
@albu-diku albu-diku changed the base branch from refactor/split-up-config-generation to edge July 22, 2024 09:28
@albu-diku albu-diku marked this pull request as ready for review July 22, 2024 09:28
@albu-diku albu-diku changed the title Break the installation defaults out as their own structure. Break installation defaults out as their own structure. Jul 22, 2024
@albu-diku albu-diku force-pushed the refactor/split-out-install-defaults branch from 24e8183 to f4198e0 Compare July 23, 2024 05:30
@jonasbardino
Copy link
Contributor

The restructure looks good and I was just about to merge when I noticed the py2 unit tests failing as described above. Should we move the _make_parameters helper to a generate_confs_params_spec in mig.shared.install and grab it from there for now?

@albu-diku
Copy link
Contributor Author

The restructure looks good and I was just about to merge when I noticed the py2 unit tests failing as described above. Should we move the _make_parameters helper to a generate_confs_params_spec in mig.shared.install and grab it from there for now?

So I considered that but actually I'd lean towards not yet - I think the "how we might wan this to be" story is still evolving and it's less churn to leave them there and follow-up once we have clearer picture (some extra context here: #83 (comment)).

@jonasbardino jonasbardino self-assigned this Jul 23, 2024
@jonasbardino
Copy link
Contributor

Not sure if this overlaps with your recent answer, but while experimenting a bit with that move into mig.shared.install I hit another snag with the py2 unit tests...
Event after make distclean I hit an ImportError: cannot import name SimpleNamespace. AFAICT it was not added in the python stdlib until 3.3 so it probably won't fly as such.
Perhaps we could use the following workaround if you really, really want to take the class-way:
https://opendev.org/x/ospurge/commit/c2d1728deedf7e2ca03e6a55f4de59d4d3ab1dde
but otherwise we'd usually just go with a good old plain dict, which seems to be all you really need here.

@albu-diku albu-diku force-pushed the refactor/split-out-install-defaults branch 2 times, most recently from cb3da6a to 6b2f337 Compare July 23, 2024 06:45
@albu-diku
Copy link
Contributor Author

Not sure if this overlaps with your recent answer, but while experimenting a bit with that move into mig.shared.install I hit another snag with the py2 unit tests... Event after make distclean I hit an ImportError: cannot import name SimpleNamespace. AFAICT it was not added in the python stdlib until 3.3 so it probably won't fly as such. Perhaps we could use the following workaround if you really, really want to take the class-way: https://opendev.org/x/ospurge/commit/c2d1728deedf7e2ca03e6a55f4de59d4d3ab1dde but otherwise we'd usually just go with a good old plain dict, which seems to be all you really need here.

Hah yep. The big reason I leaned towards SimpleNamespace as opposed to dict is such that the defaults could be consumed as attributes, but there is so little to SimpleNamespace that I just the handful lines necessary to compat.

Add tests that assert the consistency of the options that are accepted
by the generateconfs command line with the internal library function.
Additionally assert that the library routine itself matches the defaults
structure thus making the _structure_ the definitive source of truth.

Doing so highlighted the following missing command line options which
are added as of this commit:
--seafile_secret
--seafile_ccnetid
@albu-diku albu-diku force-pushed the refactor/split-out-install-defaults branch from bab43cd to ff29485 Compare October 16, 2024 18:12
@albu-diku albu-diku removed the rebased label Nov 12, 2024
@albu-diku albu-diku closed this Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants