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

Deploy Branch Status (September 2022) #32

Merged
merged 13 commits into from
Oct 6, 2022
Merged

Deploy Branch Status (September 2022) #32

merged 13 commits into from
Oct 6, 2022

Conversation

klauer
Copy link
Contributor

@klauer klauer commented Sep 7, 2022

*** Not to be merged during September ***

This PR can be used to easily see when cron-based pushes to GitHub happen, and allow us an opportunity to review changes prior to merging into master.

(Will Tyler's cron job continue to work? Hmmm) Yes, it will

@ZLLentz
Copy link
Member

ZLLentz commented Sep 9, 2022

Do we need to track down this cron job and relocate it to a new username/credentials?

@klauer
Copy link
Contributor Author

klauer commented Oct 3, 2022

Seems like the cron job persists, at least for now. We should take it over at some point...

What happened with the force push there, @ZLLentz?

... and it's October. Requesting reviews.

@ZLLentz
Copy link
Member

ZLLentz commented Oct 4, 2022

What happened with the force push there

Will was trying to follow update procedure but ended up pushing a commit here manually from a clone of the deploy branch, which broke the cron job pushes from the deploy directory until the unknown commit was overwritten.

@ZLLentz
Copy link
Member

ZLLentz commented Oct 4, 2022

Probably we need to replace the cron job with one from one of our users and push-protect this branch a bit better

@klauer
Copy link
Contributor Author

klauer commented Oct 4, 2022

Agreed - and made #33 and #34 based on your comments @ZLLentz
We can hash out details there

Copy link

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

Looks fine to me on the whole. I read more of the diff than I did last time and that's an improvement

@@ -18476,4 +20333,4 @@
"type": "pcdsdevices.happi.containers.GateValve",
"z": 870.97
}
}
}
Copy link

Choose a reason for hiding this comment

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

Should we put this newline back?

Suggested change
}
}

"ioc_type": "BaslerGigE",
"ioc_use_evr": "172.21.160.174",
"ioc_use_evr": "yes",
Copy link

Choose a reason for hiding this comment

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

This is correct given the container, but I wonder why it's not a boolean value instead of the current string enforce type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Looks like a copy-pasta error.
@christina-pino and Aron were working on this. Aron is the ioc_engineer in this entry.

Choose a reason for hiding this comment

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

NVM, I need to open my eyes.
Yeah, this would probably be better as a boolean.

@klauer
Copy link
Contributor Author

klauer commented Oct 6, 2022

Changes seem mostly OK, aside from the things @tangkong pointed out.

Merging, a bit past our deadline. Will open a new PR tomorrow

@klauer klauer merged commit e9fcbc8 into master Oct 6, 2022
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.

5 participants