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

reorganisation of constants and validation of config settings #266

Merged

Conversation

trz42
Copy link
Contributor

@trz42 trz42 commented Apr 22, 2024

This PR turned out to be larger than originally intended (just validating that required config settings are defined when bot components start). However, it reorganises / brings consistent structure to how and where string constants are defined. Hopefully, this makes the code easier to read.

  • string constants being used have been defined in the correct modules
    • when they are used they have to be prefixed with the module, hence it becomes more clear to which module they belong
  • a new file tools/cvmfs_repository.py for constants used for the settings of a CernVM-FS repository is added
  • both the event handler and job manager verify at the start that required/recommended settings are defined in 'app.cfg'

TODO

- string constants being used have been defined in the correct modules
  - when they are used they have to be prefixed with the module, hence
    it becomes more clear to which module they belong
- a new file tools/cvmfs_repository.py for constants used for the settings of a
  CernVM-FS repository is added
- both the event handler and job manager verify at the start that
  required/recommended settings are defined in 'app.cfg'
Copy link
Member

@Neves-P Neves-P left a comment

Choose a reason for hiding this comment

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

Note: I have not run the bot instance on our site with this PR yet, I have only reviewed the code.

I think it's a good change and it adds needed structure for the bot configuration which will make it easier to setup on other sites too! I've just noted a few nitpicking things which could just be me not understanding the logic, go ahead and reject if that's the case.

tools/cvmfs_repository.py Outdated Show resolved Hide resolved
eessi_bot_event_handler.py Show resolved Hide resolved
tools/job_metadata.py Show resolved Hide resolved
tools/job_metadata.py Outdated Show resolved Hide resolved
Copy link
Member

@Neves-P Neves-P left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@Neves-P Neves-P merged commit 02fd520 into EESSI:develop May 6, 2024
7 checks passed
@trz42 trz42 mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants