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

Fixes #6604: handle SIGHUP for cf-serverd #2008

Closed
wants to merge 1 commit into from

Conversation

ncharles
Copy link
Contributor

@cfengine-review-bot
Copy link

Can one of the admins verify this patch?

@basvandervlies
Copy link
Contributor

+1

GenericAgentCheckPolicy(config, true, true);
ClearRequestReloadConfig();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't this follow the same code path as below? Wouldn't it be better to do something like:

-    if (config->agent_specific.daemon.last_validated_at < validated_at)
+    if (config->agent_specific.daemon.last_validated_at < validated_at
+        || IsRequestReloadConfig())
     {
+        if (IsRequestReloadConfig())
+        {
+            ClearRequestReloadConfig();
+            validated_at = time(NULL);
+        }
         /* Rereading policies now, so update timestamp. */

or something along those lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didn't want to modify manually the validated_at, and forcing a promise reload sounded more sensible, for instance if new promises are invalid, it would still use the previous set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want, I can move the block within the
if (config->agent_specific.daemon.last_validated_at < validated_at || IsRequestReloadConfig())

@kacf
Copy link
Contributor

kacf commented Oct 8, 2014

Looks sensible to handle SIGHUP in this way though.

@jimis
Copy link
Contributor

jimis commented Oct 9, 2014

Thanks! I just commented on redmine 6604 to figure out if we really want this. Code issues can be reviewed later. :-)

@cfengine-review-bot
Copy link

Can one of the admins verify this patch?

1 similar comment
@cfengine-review-bot
Copy link

Can one of the admins verify this patch?

@kacf
Copy link
Contributor

kacf commented Mar 27, 2015

Merged with many additional fixes in d0547c2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants