Skip to content

Conversation

@craigcomstock
Copy link
Contributor

@craigcomstock craigcomstock commented Feb 20, 2025

related to management of share/GUI and not-share/GUI of two php configuration files: cfengine/masterfiles#2988

Ticket: ENT-12658
Changelog: none

@cf-bottom
Copy link

Thanks for submitting a pull request! Maybe @craigcomstock can review this?

Copy link
Member

@nickanderson nickanderson left a comment

Choose a reason for hiding this comment

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

I don't fully object to the glob in the spec file, but I do lean a bit towards full enumeration

… packages

To change the port, masterfiles can change these files which are distributed as part of the package so we must mark them as configuration files.
cfengine/masterfiles#2987

Ticket: ENT-12658
Changelog: none
Copy link
Member

@nickanderson nickanderson left a comment

Choose a reason for hiding this comment

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

You think we should mark the files in share/GUI as config files? They aren't the actual files that are used by the application.

Those files should only be modified if we have defined mpf_enable_mission_portal_docroot_sync_from_share_gui.

bundle agent update_cli_rest_server_url_config
{
  vars:
    # Both share and live versions must be changed at once since httpd will be restarted later in the same agent run.
    "mp_config_file" string => "$(cfe_internal_hub_vars.docroot)/application/config/config.php";
    "mp_share_config_file" string => "$(sys.workdir)/share/GUI/application/config/config.php";
    "regex_test_pattern" string => ".*localhost:$(cfe_internal_hub_vars.https_port).*";

  files:
    mpf_enable_mission_portal_docroot_sync_from_share_gui::
      "$(mp_share_config_file)"
        handle => "cfe_internal_edit_share_gui_mp_config",
        edit_line => change_cli_rest_server_url_port,
        if => and(
                   fileexists("$(mp_share_config_file)"),
                   islessthan(countlinesmatching("$(regex_test_pattern)", "$(mp_share_config_file)"), 1)
        );

    any::
      "$(mp_config_file)"
        handle => "cfe_internal_edit_mp_config",
        edit_line => change_cli_rest_server_url_port,
        if => and(
          fileexists("$(mp_config_file)"),
          islessthan(countlinesmatching("$(regex_test_pattern)", "$(mp_config_file)"), 1)
          );
}

I was thinking that we should not mark any of the files in share/GUI as config files.

@craigcomstock
Copy link
Contributor Author

You think we should mark the files in share/GUI as config files? They aren't the actual files that are used by the application.

Those files should only be modified if we have defined mpf_enable_mission_portal_docroot_sync_from_share_gui.

bundle agent update_cli_rest_server_url_config
{
  vars:
    # Both share and live versions must be changed at once since httpd will be restarted later in the same agent run.
    "mp_config_file" string => "$(cfe_internal_hub_vars.docroot)/application/config/config.php";
    "mp_share_config_file" string => "$(sys.workdir)/share/GUI/application/config/config.php";
    "regex_test_pattern" string => ".*localhost:$(cfe_internal_hub_vars.https_port).*";

  files:
    mpf_enable_mission_portal_docroot_sync_from_share_gui::
      "$(mp_share_config_file)"
        handle => "cfe_internal_edit_share_gui_mp_config",
        edit_line => change_cli_rest_server_url_port,
        if => and(
                   fileexists("$(mp_share_config_file)"),
                   islessthan(countlinesmatching("$(regex_test_pattern)", "$(mp_share_config_file)"), 1)
        );

    any::
      "$(mp_config_file)"
        handle => "cfe_internal_edit_mp_config",
        edit_line => change_cli_rest_server_url_port,
        if => and(
          fileexists("$(mp_config_file)"),
          islessthan(countlinesmatching("$(regex_test_pattern)", "$(mp_config_file)"), 1)
          );
}

I was thinking that we should not mark any of the files in share/GUI as config files.

I guess the thing is that the share/gui files are distributed and must be changed in one case where they are the source of truth copied to the running directories.
Since you can change the setting of whether to use share/gui or not I think we must mark these files as config files as the package manager will ideally complain that they are changed if say we specify a custom http port in augments.

@nickanderson
Copy link
Member

I guess the thing is that the share/gui files are distributed and must be changed in one case where they are the source of truth copied to the running directories. Since you can change the setting of whether to use share/gui or not I think we must mark these files as config files as the package manager will ideally complain that they are changed if say we specify a custom http port in augments.

Yes, there is policy related to editing configuration related to port change, but it's also instrumented for mpf_enable_mission_portal_docroot_sync_from_share_gui :

bundle agent update_cli_rest_server_url_config
{
  vars:
    # Both share and live versions must be changed at once since httpd will be restarted later in the same agent run.
    "mp_config_file" string => "$(cfe_internal_hub_vars.docroot)/application/config/config.php";
    "mp_share_config_file" string => "$(sys.workdir)/share/GUI/application/config/config.php";
    "regex_test_pattern" string => ".*localhost:$(cfe_internal_hub_vars.https_port).*";

  files:
    mpf_enable_mission_portal_docroot_sync_from_share_gui::
      "$(mp_share_config_file)"
        handle => "cfe_internal_edit_share_gui_mp_config",
        edit_line => change_cli_rest_server_url_port,
        if => and(
                   fileexists("$(mp_share_config_file)"),
                   islessthan(countlinesmatching("$(regex_test_pattern)", "$(mp_share_config_file)"), 1)
        );

    any::
      "$(mp_config_file)"
        handle => "cfe_internal_edit_mp_config",
        edit_line => change_cli_rest_server_url_port,
        if => and(
          fileexists("$(mp_config_file)"),
          islessthan(countlinesmatching("$(regex_test_pattern)", "$(mp_config_file)"), 1)
          );
}

So, it only targets modifying the file in share/gui if copying from share gui is enabled.

@craigcomstock
Copy link
Contributor Author

@nickanderson I do think we must mark these as configuration files as they are part of the distribution and can be changed thereby causing package manager trouble.

@nickanderson
Copy link
Member

@nickanderson I do think we must mark these as configuration files as they are part of the distribution and can be changed thereby causing package manager trouble.

I understand, and I think I still disagree based on the default configuration for master not copying those files by default.

But I think you are a better decider here than I am.

@craigcomstock
Copy link
Contributor Author

@nickanderson I do think we must mark these as configuration files as they are part of the distribution and can be changed thereby causing package manager trouble.

I understand, and I think I still disagree based on the default configuration for master not copying those files by default.

But I think you are a better decider here than I am.

RIght on. Appreciate your input. In this case it seems best to be cautious and merge so will do. 👍

@craigcomstock craigcomstock merged commit 35be912 into cfengine:master Aug 27, 2025
1 of 2 checks passed
@craigcomstock craigcomstock deleted the ENT-12658/master branch August 27, 2025 15:27
@craigcomstock
Copy link
Contributor Author

cherry picks
in 3.24.x: #1865
and 3.21.x: #1866

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants