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

Require --feeling-safe for local provisioning #3103

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

martinhoyer
Copy link
Collaborator

@martinhoyer martinhoyer commented Jul 22, 2024

Resolves #3027

Pull Request Checklist

  • implement the feature
  • write the documentation
  • extend the test coverage
  • update the specification
  • adjust plugin docstring
  • modify the json schema
  • mention the version
  • include a release note

@martinhoyer
Copy link
Collaborator Author

@happz Could you please give me a sanity check here?
I was thinking about creating some reusable decorator for this, but got a bit lost with the loggers, etc and wasn't sure how to make it universal.
If you could give me some pointers how would you like me to do this, it would be much appreciated.

@happz
Copy link
Collaborator

happz commented Jul 22, 2024

@happz Could you please give me a sanity check here? I was thinking about creating some reusable decorator for this, but got a bit lost with the loggers, etc and wasn't sure how to make it universal. If you could give me some pointers how would you like me to do this, it would be much appreciated.

Sure. I didn't think about using a decorator, seems like too much work, and then one can decorate functions and methods only, which means we would need to refactor code to be able to decorate with the "feeling safe" decorator.

What you have seems fine to me in general, just a few comments WRT implementation:

  • instead of an explicit check of environment, we could add envvar to the option in tmt/options.py. That should work out of the box, Click would then extract the value from the environment for us.

  • that would also align TMT_FEELING_SAFE with flag-like envvars for plugin options, see the note in https://tmt.readthedocs.io/en/stable/overview.html#plugin-variables - i.e. not TMT_FEELING_SAFE=true but TMT_FEELING_SAFE=1.

  • the check looks like we should have a helper function for it, to isolate local.py from the burden of __version__ and so on. Something we could re-use easily, something like assert_feeling_safe("1.38", "The 'local' provision plugin"), to check the option, the tmt version, emit the message, maybe return True if everything was fine so the caller could continue or quit. Something we can cover with a unit test, e.g.:

    def assert_feeling_safe(version: str, subject: str) -> bool:
        if self.is_feeling_safe:
            return True
    
        if tmt.__version__ < version:
            self.warn(f"{subject} will require '--feeling-safe' option from version {version}.")
            return True
    
        self.fail(f"{subject} requires '--feeling-safe' option")
        return False

@martinhoyer
Copy link
Collaborator Author

Thanks a lot @happz, that was super helpful feedback. I've tried to incorporate your suggestions, but haven't had time to test it properly. Also, the unit test is kinda work in progress - I need to go through existing tests to see how things are done here.

@martinhoyer martinhoyer marked this pull request as ready for review July 24, 2024 10:49
@martinhoyer martinhoyer requested a review from happz July 30, 2024 11:46
@martinhoyer martinhoyer added this to the 1.35 milestone Jul 31, 2024
@happz happz added step | provision Stuff related to the provision step ci | full test Pull request is ready for the full test execution labels Jul 31, 2024
tmt/steps/__init__.py Outdated Show resolved Hide resolved
@happz happz added the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Jul 31, 2024
@happz
Copy link
Collaborator

happz commented Aug 1, 2024

Unrelated failures, merging.

@happz happz merged commit 908d4c4 into teemtee:main Aug 1, 2024
19 of 20 checks passed
The-Mule pushed a commit to The-Mule/tmt that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci | full test Pull request is ready for the full test execution status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. step | provision Stuff related to the provision step
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow local provision only with the --feeling-safe option
3 participants