Skip to content
This repository has been archived by the owner on Aug 9, 2024. It is now read-only.

feat!: overhaul slurmrestd charm API #19

Merged

Conversation

jamesbeedy
Copy link
Contributor

@jamesbeedy jamesbeedy commented Jun 9, 2024

Please reference the Slurm Charms Change Summary Document for an in-depth explanation of these changes.

Changes include:

  • remove jinja2 and replace with Path().write_text()
  • remove slurm-ops-manager 3rd party dep and replace with local slurmrestd_ops.py
  • consolidate the yaml files into a single charmcraft.yaml
  • rename interface slurmrestd -> slurmctld
  • cleanup from PR review
  • remove unused code
  • addeded logging
  • simplified api
  • reduced code footprint
  • consolidated yaml files into charmcraft.yaml
  • added type checking

@jamesbeedy jamesbeedy force-pushed the slurm_config_editor_preparation branch from b843a55 to f414f01 Compare June 9, 2024 19:03
@NucciTheBoss NucciTheBoss self-requested a review June 19, 2024 20:28
Copy link
Member

@NucciTheBoss NucciTheBoss left a comment

Choose a reason for hiding this comment

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

Overall, looks great to me! Just a couple of comments and nits

README.md Outdated Show resolved Hide resolved
src/slurmrestd_ops.py Outdated Show resolved Hide resolved
src/slurmrestd_ops.py Outdated Show resolved Hide resolved
src/templates/slurm.conf.tmpl Outdated Show resolved Hide resolved
src/slurmrestd_ops.py Outdated Show resolved Hide resolved
src/slurmrestd_ops.py Outdated Show resolved Hide resolved
src/slurmrestd_ops.py Outdated Show resolved Hide resolved
src/slurmrestd_ops.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
src/slurmrestd_ops.py Show resolved Hide resolved
@NucciTheBoss
Copy link
Member

Hey @jamesbeedy 👋

Similar to the slurmd operator, could you please apply the changes you made to the slurmdbd and slurmctld operators here as well before merging? We can focus on feature refinement in follow-on pull requests 😄

@jamesbeedy jamesbeedy force-pushed the slurm_config_editor_preparation branch from 14237a7 to 0367e5f Compare June 28, 2024 16:04
Changes include:
* remove jinja2 and replace with `Path().write_text()`
* remove `slurm-ops-manager` 3rd party dep and replace with local `slurmrestd_ops.py`
* consolidate the yaml files into a single `charmcraft.yaml`
* rename interface slurmrestd -> slurmctld
* cleanup from PR review
* remove unused code
* addeded logging
* simplified api
* reduced code footprint
* consolidated yaml files into charmcraft.yaml
* added type checking
@jamesbeedy
Copy link
Contributor Author

@NucciTheBoss g2g on my end

@jamesbeedy jamesbeedy force-pushed the slurm_config_editor_preparation branch from 1b1f0a6 to 0367e5f Compare June 28, 2024 16:10
Copy link
Member

@NucciTheBoss NucciTheBoss left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your work @jamesbeedy 🎉

@NucciTheBoss NucciTheBoss changed the title James Longdev feat!: overhaul slurmrestd charm API Jun 28, 2024
@NucciTheBoss NucciTheBoss merged commit 81c9d6c into charmed-hpc:main Jun 28, 2024
4 of 5 checks passed
@jamesbeedy jamesbeedy deleted the slurm_config_editor_preparation branch July 1, 2024 14:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants