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

ENH: check for github early to avoid long timeouts, decrease usage of git clone #199

Merged
merged 6 commits into from
Sep 9, 2024

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Sep 6, 2024

Description

This should be the second-to-last PR from me about this script from the Jira epic.

Main goal: make ioc-deploy succeed and fail faster, avoiding slow commands and long timeouts.

Main changes:

  • Before a deploy action, ping github as a quick connectivity check.
  • Check tags, auth, and repo availability all at once using git ls-remote one time instead of through multiple git clone commands.
  • Rearrange logic in get_deploy_info, finalize_name, and finalize_tag to facilitate the above.

Incidental changes:

  • Normalize DeployInfo to only use str, not None because it simplified some implementation details
  • Rearrange some of the docstrings to match where code has moved
  • Allow providing --path-override by itself during the deploy routines, which previously would error out. This fell out naturally from the rest of the refactor and I didn't realize it hadn't been supported previously.

Motivation and Context

Full details at https://jira.slac.stanford.edu/browse/ECS-6096

If you git clone from a server like las-console or rix-control, one of the following will happen:

  • If you have psproxy configured, you will be prompted for your password separately on each clone.
  • If you do not have psproxy configured, you will time out after 2 minutes.

Both of these cases are avoided by this PR. I wanted to avoid setting timeouts on git clone because the command can take an arbitrarily long amount of time based on the network situation and file sizes. For the same reason, I wanted to avoid calling git clone multiple times if it could be avoided.

For example: cloning the gigECam ioc can often be slow, so I'd like this script to only do it once.

How Has This Been Tested?

Interactively, extensively

Where Has This Been Documented?

Only in this PR and release notes. The expected results of running ioc-deploy remains unchanged, it should just succeed and fail faster.

Copy link
Contributor

@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.

This looks like a straight improvement to me. This is on the cusp of deserving its own repo / package, this file is quite long..

Whoops about the --path-override option. Should have seen that as a reviewer 😞

Copy link
Contributor

@slactjohnson slactjohnson left a comment

Choose a reason for hiding this comment

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

LGTM!

@ZLLentz
Copy link
Member Author

ZLLentz commented Sep 9, 2024

This is on the cusp of deserving its own repo / package, this file is quite long..

The next time this needs a sizable scope increase, I think the first step is to create an ioc_deploy subfolder and make a mini ioc deploy module within engineering_tools, to at least modularize the logic without gaining the weight of formal packaging. If that becomes a bit bigger or we end up adding non-standard-library dependencies then I'd want to tear it out into its own repo.

@ZLLentz ZLLentz merged commit 0cc8242 into pcdshub:master Sep 9, 2024
2 checks passed
@ZLLentz ZLLentz deleted the enh_clone_perf branch September 9, 2024 17:36
@tangkong
Copy link
Contributor

tangkong commented Sep 9, 2024

I wondered if ghapi would have saved you some work, but being part of engineering tools makes me value minimal dependencies a bit higher

@ZLLentz
Copy link
Member Author

ZLLentz commented Sep 9, 2024

I'm intentionally avoiding ghapi sorts of things here anyway (despite the power/convenience) because of the anonymous access api rate limits. I don't want people to have this script error out because someone else used psbuild-rhel7-01 earlier to test their other ghapi script and forgot to authenticate.

@ZLLentz
Copy link
Member Author

ZLLentz commented Sep 9, 2024

The other problem with using ghapi is that you'd actually need to make a token for non-public repos which is a bit much for what this script is trying to do, to that end I tried to do everything through the ssh auth only, which everyone needs to have set up to use github at all.

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