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

fix: Shorten the time before API call #31

Merged
merged 2 commits into from
May 22, 2024

Conversation

rfay
Copy link
Member

@rfay rfay commented May 17, 2024

The current 55 day timeout before we hit the API seems to be a little long, as GitHub warns before 55 days. We can shorten it, here to 45.

But I'm not sure why we don't just use 1 here instead, not sure it would do any harm to hit the API daily.

The current 55 day timeout before we hit the API seems to be a little long, as GitHub warns before 55 days. We can shorten it, here to 45.

But I'm not sure why we don't just use `1` here instead, not sure it would do any harm to hit the API daily.
@julienloizelet
Copy link
Collaborator

julienloizelet commented May 18, 2024

Thanks for this PR.

I'd rather wait for answers on this time_elapsed value.

In the end, I think we could set "0" as the default entry (this should make one call each time).

@gautamkrishnar
Copy link

gautamkrishnar commented May 18, 2024

Thanks a lot for the PR and issue @rfay. Thanks for debugging. @julienloizelet 👍🏻.

This looks good to me to be used with my project. Some more docs changes are required on my project, I will make those myself. I will also make this default value in mine.

Update: released a version that uses this value as default: https://github.com/gautamkrishnar/keepalive-workflow/releases/tag/2.0.3

@julienloizelet
Copy link
Collaborator

@rfay ,
so now that we have all the answers, we have to decide.
Should we keep "45" in this PR or should we set "0" ?
In my opinion, "0" is a good move (if an user want to change, he still can as it is a configurable input).

With "0", the api call will be done on every run.

@rfay
Copy link
Member Author

rfay commented May 20, 2024

0 seems OK to me.

@julienloizelet julienloizelet self-requested a review May 20, 2024 23:36
Copy link
Collaborator

@julienloizelet julienloizelet left a comment

Choose a reason for hiding this comment

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

As we discussed, we could set "0" here, as a default value.

To be consistent with keepalive original workflow, we should set the string "0" (and not the number 0) because the original workflow expects a string as input.

(In my opinion, it was an already an error to set 55 and not "55" as default value).

action.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@julienloizelet julienloizelet left a comment

Choose a reason for hiding this comment

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

Thanks.

I will merge and make a new release asap (tomorrow, I guess)

@julienloizelet julienloizelet merged commit ca4409b into main May 22, 2024
21 checks passed
@rfay
Copy link
Member Author

rfay commented May 23, 2024

I'm not sure, but I think this may not have worked out right. I'm seeing test failures on stable (not latest) in a couple of repos, where the tests have obviously succeeded:

@julienloizelet
Copy link
Collaborator

I'm not sure, but I think this may not have worked out right. I'm seeing test failures on stable (not latest) in a couple of repos, where the tests have obviously succeeded:

* https://github.com/justafish/ddev-drupal-core-dev/actions/runs/9213831558/job/25348604861?pr=33

* https://github.com/ddev/ddev-solr/actions/runs/9204758550

Very strange, indeed: it appears to works well for most of the repos.

I've tried a couple of tests and all was successful.

For the solr repo, there is a permission issue; maybe it can explain the error.

For the ddev-drupal-core-dev repo, there is no obvious problem: I suspect a branch protection rule, or action permission settings, but, at least, there should be a more explicit error message in this case.

I'll ask @gautamkrishnar if he has any idea what might be the root cause.

@julienloizelet
Copy link
Collaborator

I've created an issue : gautamkrishnar/keepalive-workflow#42

Thanks

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