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

fix: save config and env on plugins enable/disable #957

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Conversation

regisb
Copy link
Contributor

@regisb regisb commented Dec 8, 2023

This replaces #917

before this, after enabling/disabling any plugins we should re-generate all files with tutor config save.
@regisb regisb changed the base branch from master to nightly December 8, 2023 11:01
@regisb regisb force-pushed the fix/save-configs branch 2 times, most recently from e34734f to 55d466e Compare December 8, 2023 11:40
Also, update docs on `tutor config save`.

Note that we had to fix an issue where the plugin unload callback was
being called too late.
@regisb regisb merged commit 6582e3a into nightly Dec 8, 2023
1 check passed
@regisb regisb deleted the fix/save-configs branch December 8, 2023 11:47
fghaas added a commit to hastexo/tutor-contrib-s3 that referenced this pull request Dec 14, 2023
Until Tutor 17, just enabling a plugin with "tutor plugin enable"
would not recreate the configuration, meaning the
tests/tutor-sandbox/env directory would remain empty.

As of overhangio/tutor#957, however,
"tutor plugin enable" does automatically also run "tutor config save",
generating a full Tutor configuration.

This means that unless configured otherwise, with Tutor 17 our flake8
checks would test code generated by Tutor, rather than just that
generated by our plugin.

Thus, to make our tests not break once we bump our dependencies to
support Tutor 17, exclude the tests/tutor-sandbox/env checks from
flake8.
fghaas added a commit to fghaas/tutor-contrib-backup that referenced this pull request Dec 14, 2023
Until Tutor 17, just enabling a plugin with "tutor plugin enable"
would not recreate the configuration, meaning the
tests/tutor-sandbox/env directory would remain empty.

As of overhangio/tutor#957, however,
"tutor plugin enable" does automatically also run "tutor config save",
generating a full Tutor configuration.

This means that unless configured otherwise, with Tutor 17 our flake8
checks would test code generated by Tutor, rather than just that
generated by our plugin.

Thus, to make our tests not break once we bump our dependencies to
support Tutor 17, exclude the tests/tutor-sandbox/env checks from
flake8.
fghaas added a commit to fghaas/tutor-contrib-retirement that referenced this pull request Dec 14, 2023
Until Tutor 17, just enabling a plugin with "tutor plugin enable"
would not recreate the configuration, meaning the
tests/tutor-sandbox/env directory would remain empty.

As of overhangio/tutor#957, however,
"tutor plugin enable" does automatically also run "tutor config save",
generating a full Tutor configuration.

This means that unless configured otherwise, with Tutor 17 our flake8
checks would test code generated by Tutor, rather than just that
generated by our plugin.

Thus, to make our tests not break once we bump our dependencies to
support Tutor 17, exclude the tests/tutor-sandbox/env checks from
flake8.
fghaas added a commit to fghaas/tutor-contrib-enrollmentreports that referenced this pull request Dec 14, 2023
Until Tutor 17, just enabling a plugin with "tutor plugin enable"
would not recreate the configuration, meaning the
tests/tutor-sandbox/env directory would remain empty.

As of overhangio/tutor#957, however,
"tutor plugin enable" does automatically also run "tutor config save",
generating a full Tutor configuration.

This means that unless configured otherwise, with Tutor 17 our flake8
checks would test code generated by Tutor, rather than just that
generated by our plugin.

Thus, to make our tests not break once we bump our dependencies to
support Tutor 17, exclude the tests/tutor-sandbox/env checks from
flake8.
fghaas added a commit to fghaas/tutor-contrib-openstack that referenced this pull request Dec 14, 2023
Until Tutor 17, just enabling a plugin with "tutor plugin enable"
would not recreate the configuration, meaning the
tests/tutor-sandbox/env directory would remain empty.

As of overhangio/tutor#957, however,
"tutor plugin enable" does automatically also run "tutor config save",
generating a full Tutor configuration.

This means that unless configured otherwise, with Tutor 17 our flake8
checks would test code generated by Tutor, rather than just that
generated by our plugin.

Thus, to make our tests not break once we bump our dependencies to
support Tutor 17, exclude the tests/tutor-sandbox/env checks from
flake8.
fghaas added a commit to fghaas/tutor-contrib-webhook-receiver that referenced this pull request Dec 14, 2023
Until Tutor 17, just enabling a plugin with "tutor plugin enable"
would not recreate the configuration, meaning the
tests/tutor-sandbox/env directory would remain empty.

As of overhangio/tutor#957, however,
"tutor plugin enable" does automatically also run "tutor config save",
generating a full Tutor configuration.

This means that unless configured otherwise, with Tutor 17 our flake8
checks would test code generated by Tutor, rather than just that
generated by our plugin.

Thus, to make our tests not break once we bump our dependencies to
support Tutor 17, exclude the tests/tutor-sandbox/env checks from
flake8.
fghaas added a commit to fghaas/tutor-contrib-hastexo that referenced this pull request Dec 14, 2023
Until Tutor 17, just enabling a plugin with "tutor plugin enable"
would not recreate the configuration, meaning the
tests/tutor-sandbox/env directory would remain empty.

As of overhangio/tutor#957, however,
"tutor plugin enable" does automatically also run "tutor config save",
generating a full Tutor configuration.

This means that unless configured otherwise, with Tutor 17 our flake8
checks would test code generated by Tutor, rather than just that
generated by our plugin.

Thus, to make our tests not break once we bump our dependencies to
support Tutor 17, exclude the tests/tutor-sandbox/env checks from
flake8.
@fghaas
Copy link
Contributor

fghaas commented Dec 14, 2023

Please note: plugin authors may need to be aware of the impact this change may have on their unit and integration tests. See the PRs referenced from here.

@regisb
Copy link
Contributor Author

regisb commented Dec 14, 2023

@fghaas Would it make sense to add a --no-save-env (or similar) option to tutor plugins enable/disable?

@fghaas
Copy link
Contributor

fghaas commented Dec 14, 2023

Good question. Most of the time, users would want automatic config save, particularly if the plugin requires a config.yml variable to be initialized. So I think Tutor does the "right" thing by default in v17, except if we're applying an excessively strict interpretation of POLA.

Would a toggle to turn it off be useful, or would it be more likely to be a footgun? I really don't know.

@regisb
Copy link
Contributor Author

regisb commented Dec 14, 2023

Let's do nothing for now. If you (or anyone else) later think it's important we can decide to add a toggle.

@fghaas
Copy link
Contributor

fghaas commented Dec 14, 2023

That works for me!

mrtmm pushed a commit to mrtmm/tutor-contrib-registry that referenced this pull request Jan 11, 2024
Until Tutor 17, just enabling a plugin with "tutor plugin enable"
would not recreate the configuration, meaning the
tests/tutor-sandbox/env directory would remain empty.

As of overhangio/tutor#957, however,
"tutor plugin enable" does automatically also run "tutor config save",
generating a full Tutor configuration.

This means that unless configured otherwise, with Tutor 17 our flake8
checks would test code generated by Tutor, rather than just that
generated by our plugin.

Thus, to make our tests not break once we bump our dependencies to
support Tutor 17, exclude the tests/tutor-sandbox/env checks from
flake8.
mrtmm pushed a commit to mrtmm/tutor-contrib-registry that referenced this pull request Jan 11, 2024
Until Tutor 17, just enabling a plugin with "tutor plugin enable"
would not recreate the configuration, meaning the
tests/tutor-sandbox/env directory would remain empty.

As of overhangio/tutor#957, however,
"tutor plugin enable" does automatically also run "tutor config save",
generating a full Tutor configuration.

This means that unless configured otherwise, with Tutor 17 our flake8
checks would test code generated by Tutor, rather than just that
generated by our plugin.

Thus, to make our tests not break once we bump our dependencies to
support Tutor 17, exclude the tests/tutor-sandbox/env checks from
flake8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants