Skip to content

traffic_ctl: Display YAML format when --records option is set (instead of the legacy records.config style)#11650

Merged
brbzull0 merged 2 commits intoapache:masterfrom
brbzull0:traffic_ctl_records_display_yaml
Aug 14, 2024
Merged

traffic_ctl: Display YAML format when --records option is set (instead of the legacy records.config style)#11650
brbzull0 merged 2 commits intoapache:masterfrom
brbzull0:traffic_ctl_records_display_yaml

Conversation

@brbzull0
Copy link
Copy Markdown
Contributor

@brbzull0 brbzull0 commented Aug 5, 2024

This PR changes the traffic_ctl config output when --records is requested.
--records was used to display the output of a record in the records.config configuration style:

$ traffic_ctl config get  proxy.config.diags.show_location proxy.config.diags.logfile_perm proxy.config.diags.logfile.filename --records

CONFIG proxy.config.diags.show_location INT 2
CONFIG proxy.config.diags.logfile_perm STRING rw-r--r--
CONFIG proxy.config.diags.logfile.filename STRING diags.log

As we do not support records.config style anymore, it now make sense to have just YAML, so the same command will now display:

$ traffic_ctl config get  proxy.config.diags.show_location proxy.config.diags.logfile_perm proxy.config.diags.logfile.filename --records
records:
  diags:
    logfile:
      filename: diags.log
    logfile_perm: rw-r--r--
    show_location: 2

For things like traffic_ctl config diff --records it will add the default value as standard YAML comment.

Note: This is the main reason why this PR uses YAML::Emitter and not just the YAML::Node to build up the yaml doc, it would have been very easy just to use the YAML::Node but it cannot keep track of the comments which for things like the diff option is key.

$ traffic_ctl config diff --records
records:
  diags:
    debug:
      tags: quic|udp  # default: http|dns
    show_location: 2  # default: 1
  http:
    server_ports: 8080 4443:ssl 4443:quic  # default: 8080 8080:ipv6
  quic:
    initial_max_streams_bidi_in: 5  # default: 100
    initial_max_streams_bidi_out: 100000  # default: 100
  udp:
    enable_gso: 0  # default: 1
    threads: 1  # default: 0

if no --records option is passed, the behavior remain unchanged.

fixes: #11607

It make sense to display YAML instead of the legacy format in case this
out wants to be used as input for ATS.
A new bunch of function are added only for traffic_ctl to build up yaml
from the record's name.
This code uses YAML::Emitter so we can add comments to the yaml doc,
this is essential as we are including things like defaults values in
Each field, this can only be accomplished with Emitters.
@brbzull0 brbzull0 added Tools traffic_ctl traffic_ctl related work. labels Aug 5, 2024
@brbzull0 brbzull0 added this to the 10.1.0 milestone Aug 5, 2024
@brbzull0 brbzull0 self-assigned this Aug 5, 2024
@brbzull0
Copy link
Copy Markdown
Contributor Author

brbzull0 commented Aug 5, 2024

Will fix CI: Fixed

../src/traffic_ctl/PrintUtils.h:85:1: error: 'const char* rec_sourceof(int)' defined but not used [-Werror=unused-function]
   85 | rec_sourceof(int rec_source)
      | ^~~~~~~~~~~~
../src/traffic_ctl/PrintUtils.h:73:1: error: 'const char* rec_labelof(int)' defined but not used [-Werror=unused-function]
   73 | rec_labelof(int rec_class)
      | ^~~~~~~~~~~
../src/traffic_ctl/PrintUtils.h:42:1: error: 'const char* rec_updateof(int)' defined but not used [-Werror=unused-function]
   42 | rec_updateof(int rec_updatetype)
      | ^~~~~~~~~~~~
../src/traffic_ctl/PrintUtils.h:29:1: error: 'const char* rec_accessof(int)' defined but not used [-Werror=unused-function]
   29 | rec_accessof(int rec_access)

@bryancall bryancall requested a review from cmcfarlen August 5, 2024 22:05
@brbzull0 brbzull0 marked this pull request as ready for review August 7, 2024 09:42
Copy link
Copy Markdown
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

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

Worth adding a test or two?

@brbzull0
Copy link
Copy Markdown
Contributor Author

Worth adding a test or two?

definitely yes. I'll add them.
Thanks Chris.

@brbzull0
Copy link
Copy Markdown
Contributor Author

Worth adding a test or two?

@cmcfarlen : done -> #11740

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

traffic_ctl : update --records output

3 participants