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

Added motor and ipac modules #11

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Added motor and ipac modules #11

merged 1 commit into from
Sep 14, 2023

Conversation

guirodrigueslima
Copy link
Contributor

No description provided.

@ericonr ericonr requested a review from henriquesimoes August 22, 2023 20:52
Copy link
Collaborator

@henriquesimoes henriquesimoes left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I skimmed through the configuration and found somethings I think that might no be needed. Having the simplest configuration helps us maintaining it later.

Something you might not be familiar with (as I wasn't in the beginning) is to document the decisions you made in the commit messages (there is no need to document what you did, though, since this is already in the commit itself). I see you added no description to your commit, however there are several things I wasn't sure why you configured the way you did.

I think it can be interesting to mention why you have added IPAC module in the commit message. It seems to be a module required only for motorHytec.

You should also edit your commit instead of adding new commits patching your changes. Let me know if you have any questions regarding this. You should also follow the commit title style we have been using:

  • start with ioc:, base: or ci: based on the part of the repository you are changing
  • use imperative instead of past tense (e.g. add)
  • end with a period (.) (That's really an Érico stuff. Haha)

Consistency helps us finding what we want in the future.

base/.env Outdated Show resolved Hide resolved
base/install_motor.sh Show resolved Hide resolved
base/install_motor.sh Outdated Show resolved Hide resolved
base/install_motor.sh Outdated Show resolved Hide resolved
base/install_motor.sh Outdated Show resolved Hide resolved
base/install_motor.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@henriquesimoes henriquesimoes left a comment

Choose a reason for hiding this comment

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

Commit message can (and should) be more descriptive. I'd expect something like:

base: add support for motor.

Similarly to areaDetector, a separate script has been used to configure motor
module and its submodules due to its different setup.

IOCs in the support module are not built, since we currently do not use any of
them exactly the same way as they are configured in the community repositories.
This prevents the image size from increasing with unused build artifacts.

IPAC module is built to support `motorHytec`. This way, all motor modules
provided by the motor (super)module are supported.

base/Dockerfile Outdated Show resolved Hide resolved
base/install_modules.sh Outdated Show resolved Hide resolved
Similarly to areaDetector, a separate script has been used to configure motor
module and its submodules due to its different setup.

IOCs in the support module are not built, since we currently do not use any of
them exactly the same way as they are configured in the community repositories.
This prevents the image size from increasing with unused build artifacts.

IPAC module is built to support 'motorHytec'. This way, all motor modules
provided by the motor (super)module are supported.
Copy link
Member

@ericonr ericonr left a comment

Choose a reason for hiding this comment

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

Pending @henriquesimoes 's final review, I'm okay with merging this.

Thanks for the great work!

Copy link
Collaborator

@henriquesimoes henriquesimoes left a comment

Choose a reason for hiding this comment

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

It's fine by me as well. Thanks for working on this, @guirodrigueslima.

@henriquesimoes henriquesimoes merged commit 4ba9c44 into cnpem:main Sep 14, 2023
@guirodrigueslima guirodrigueslima deleted the motor branch October 27, 2023 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants