search

cnpem / epics-in-docker

Container images with EPICS and modules
5 stars 2 forks source link

Supporting statically linked IOCs #75

Closed ericonr closed 1 month ago

ericonr commented 2 months ago

Isn't better to squash this commit with the previous, keeping the changes tracking with the change itself?

I'm not sure. The change is a combination of all the other commits in the PR, IMO. Even if the last commit was the one to tie everything together :thinking:

henriquesimoes commented 2 months ago

Isn't better to squash this commit with the previous, keeping the changes tracking with the change itself?

I'm not sure. The change is a combination of all the other commits in the PR, IMO. Even if the last commit was the one to tie everything together 🤔

There is one thing that I don't like about letting them in the same commit: when reverting a commit, you need to be careful not to revert the changelog. Thus, this is necessarily partial revert. But you should probably also specify in the log that you did this, so not a big deal.

On the other hand, it is pretty handy for blaming changes.

For this case, I also understand this a whole PR change that is being described. So I'm okay with it as is.

ericonr commented 2 months ago

@henriquesimoes I didn't copy the directory inside the container as we had discussed, for the reasons laid out in the commit message. Let me know what you think.

ericonr commented 2 months ago

Assuming we merge #75 , this will require an update :)

henriquesimoes commented 2 months ago

Assuming we merge https://github.com/cnpem/epics-in-docker/pull/75 , this will require an update :)

You mean #77, right? We may do the other way around if you prefer.

ericonr commented 2 months ago

Assuming we merge #75 , this will require an update :)

You mean #77, right? We may do the other way around if you prefer.

Oops, yeah. Thanks :)

I've already rebased #77 locally on top of this, so I'm pending towards merging this one first.

gustavosr8 commented 1 month ago

WDYT about adding bash to the following in e7eeebc?

https://github.com/cnpem/epics-in-docker/blob/b27a3b044e37fffafb1a1910651d5354bf9166d5/README.md?plain=1#L29-L31