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

Allow no-validate parameter for docker registry #23

Conversation

ranesagar
Copy link
Contributor

@ranesagar ranesagar commented May 12, 2022

@ranesagar
Copy link
Contributor Author

@abhinaybyrisetty - Can you please review?

@abhinaybyrisetty
Copy link
Contributor

Yeah, this change looks good. But, skipping validation shouldn't be recommended unless extremely necessary. Perhaps, making client-timeout as configurable is a better option than skipping validation.

Copy link
Contributor

@abhinaybyrisetty abhinaybyrisetty left a comment

Choose a reason for hiding this comment

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

I'm afraid these changes won't fully solve the problem as validation will be done during hal deploy apply as well

@Badbond
Copy link

Badbond commented May 18, 2022

Thanks for picking this up so fast 🙏

Perhaps, making client-timeout as configurable is a better option than skipping validation.

AFAIK, there is no such option. A ticket was created for this but went stale.

I'm afraid these changes won't fully solve the problem as validation will be done during hal deploy apply as well

@abhinaybyrisetty Fortunately this command does have parameters to configure timeout behavior such as --wait-for-completion and --wait-for-completion-timeout-minutes. I believe it also has a longer default timeout (but couldn't find official resources on this). Luckily the chart already allows to pass additional parameters to this command to do so.

@abhinaybyrisetty
Copy link
Contributor

Hi @Badbond

There is an option to pass along with hal config command --client-timeout-millis which will allow us to override timeout. Did you try it?

@Badbond
Copy link

Badbond commented May 19, 2022

Thanks for pointing it out @abhinaybyrisetty, I did not see it before. I did just try it but unfortunately it looks like it is not used during validation (executed in Halyard pod):

$ time hal config provider docker-registry account edit dockerhub --address index.docker.io \ 
    <account settings & --repositories> \
   --client-timeout-millis 600000
+ Get current deployment
  Success
+ Get dockerhub account
  Success
<-^*_ Edit the dockerhub account>
  Timed out
Validation in Global:
! ERROR Unexpected exception:
  DaemonTaskInterrupted(interruptedTime=1652944810613, message=Task interrupted
  without cause at Thu May 19 07:20:10 GMT 2022)

- Failed to edit account dockerhub for provider dockerRegistry.
  Task killed because it was taking too long.

real	1m8.203s
user	0m5.512s
sys	0m0.523s

@Badbond
Copy link

Badbond commented Jun 1, 2022

@abhinaybyrisetty did you have some time to check out my findings? Is there is anything I can do to help in this PR?

@abhinaybyrisetty
Copy link
Contributor

@Badbond then I think, having the option to skip validation is the more convenient at the moment. Let's submit that.

@abhinaybyrisetty
Copy link
Contributor

@ranesagar I made some changes to the folder structure and added a github action to automate release. You might want to rebase the code.

@ranesagar ranesagar force-pushed the add-no-validate-parameter-to-docker-registry branch from a6b4e3e to 4bafcd7 Compare June 3, 2022 15:47
@ranesagar
Copy link
Contributor Author

@abhinaybyrisetty - PTAL

@Badbond
Copy link

Badbond commented Jun 28, 2022

Thanks for rebasing @ranesagar. @abhinaybyrisetty did you have time to check this one out? Anything I can help with?

@abhinaybyrisetty
Copy link
Contributor

LGTM, please bump the chart version to automatically publish the chart to helm repo.

@ranesagar
Copy link
Contributor Author

@abhinaybyrisetty - PTAL

Copy link
Contributor

@abhinaybyrisetty abhinaybyrisetty left a comment

Choose a reason for hiding this comment

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

LGTM

@abhinaybyrisetty abhinaybyrisetty merged commit 32368c9 into OpsMx:main Jun 30, 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.

3 participants