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

refactor: reduce usage of autogenerated getters, part 2 #236

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

ee7
Copy link
Contributor

@ee7 ee7 commented Mar 12, 2024

Issue

#214

Description

Continue from 66a5bb4, but operate on the top-level fields of ChalkConfig that have a simpler type.

With this PR, running the following command in the repo root no longer returns any matches:

git grep 'chalkConfig.get' 

These also don't return any match:

git grep 'get(chalkConfig' 
git grep 'get chalkConfig' 

ChalkConfig currently gets defined as the below, with added comments and newlines to clarify the current status. Essentially, the remaining fields are getoptsObjs and those of type OrderedTableRef.

type ChalkConfig* = ref object
  `@@attrscope@@`*: AttrScope
  # The below 12 fields will be handled later
  getoptsObjs*: GetoptsType
  keyspecs*: OrderedTableRef[string, KeySpec]
  plugins*: OrderedTableRef[string, PluginSpec]
  sinks*: OrderedTableRef[string, SinkSpec]
  sinkConfs*: OrderedTableRef[string, SinkConfigObj]
  auths*: OrderedTableRef[string, AuthSpec]
  authConfs*: OrderedTableRef[string, AuthConfigObj]
  markTemplates*: OrderedTableRef[string, MarkTemplate]
  reportTemplates*: OrderedTableRef[string, ReportTemplate]
  outputConfigs*: OrderedTableRef[string, OutputConfig]
  reportSpecs*: OrderedTableRef[string, ReportSpec]
  tools*: OrderedTableRef[string, ToolInfo]

  # The below 8 fields were handled by #235
  attestationConfig*: AttestationConfig
  extractConfig*: ExtractConfig
  dockerConfig*: DockerConfig
  execConfig*: ExecConfig
  loadConfig*: LoadConfig
  envConfig*: EnvConfig
  srcConfig*: SrcConfig
  cloudProviderConfig*: CloudProviderConfig

  # The below 2 fields will be handled later
  techStackRules*: OrderedTableRef[string, TechStackRule]
  linguist_languages*: OrderedTableRef[string, LinguistLanguage]

  # All the below fields should be handled by this PR
  config_path*: seq[string]
  config_filename*: string
  valid_chalk_command_names*: seq[string]
  valid_chalk_commands*: seq[string]
  ignore_when_normalizing*: seq[string]
  default_command*: Option[string]
  selected_command*: string
  color*: Option[bool]
  log_level*: string
  chalk_log_level*: string
  virtual_chalk*: bool
  zip_extensions*: seq[string]
  pyc_extensions*: seq[string]
  con4m_pinpoint*: bool
  chalk_debug*: bool
  cache_fd_limit*: int
  publish_audit*: bool
  report_total_time*: bool
  audit_location*: string
  audit_file_size*: Con4mSize
  log_search_path*: seq[string]
  artifact_search_path*: seq[string]
  default_tmp_dir*: Option[string]
  always_try_to_sign*: bool
  inform_if_cant_sign*: bool
  use_transparency_log*: bool
  use_signing_key_backup_service*: bool
  signing_key_backup_service_url*: string
  signing_key_backup_service_auth_config_name*: string
  signing_key_backup_service_timeout*: Con4mDuration
  signing_key_location*: string
  ignore_patterns*: seq[string]
  load_external_config*: bool
  load_embedded_config*: bool
  run_sbom_tools*: bool
  run_sast_tools*: bool
  recursive*: bool
  docker_exe*: Option[string]
  chalk_contained_items*: bool
  show_config*: bool
  ktype_names*: seq[string]
  use_report_cache*: bool
  report_cache_location*: string
  report_cache_lock_timeout_sec*: int
  force_output_on_reporting_fails*: bool
  env_always_show*: seq[string]
  env_never_show*: seq[string]
  env_redact*: seq[string]
  env_default_action*: string
  aws_iam_role*: Option[string]
  skip_command_report*: bool
  skip_summary_report*: bool
  symlink_behavior*: string
  install_completion_script*: bool
  use_pager*: bool
  crashoverride_usage_reporting_url*: string
  crashoverride_workspace_id*: string
  use_tech_stack_detection*: bool

Refs: #214

Testing

Hopefully the current tests are sufficient.

Base automatically changed from ee7/avoid-c4autoconf-getters to main March 28, 2024 15:47
ee7 added 3 commits March 28, 2024 17:27
Allow the run_management module to use the get procs. We can't make
run_management import the config module because the latter already
imports run_management, and Nim doesn't support cyclic imports yet.

The config module already exports run_management, so this commit doesn't
affect other modules.
These changes previously appeared in src/attestation.nim, which was
removed.
@ee7 ee7 force-pushed the ee7/avoid-c4autoconf-getters-part-2 branch from 3d4efd7 to 4f9a7c4 Compare March 28, 2024 16:28
@ee7 ee7 marked this pull request as ready for review March 28, 2024 16:43
@ee7 ee7 requested a review from viega as a code owner March 28, 2024 16:43
@ee7 ee7 requested review from miki725 and viega and removed request for viega March 28, 2024 16:43
@ee7 ee7 merged commit 4157b58 into main Apr 10, 2024
3 checks passed
@ee7 ee7 deleted the ee7/avoid-c4autoconf-getters-part-2 branch April 10, 2024 11:26
@ee7 ee7 changed the title refactor: avoid c4autoconf getters, part 2 refactor: reduce usage of autogenerated getters, part 2 Apr 10, 2024
@ee7
Copy link
Contributor Author

ee7 commented Apr 10, 2024

Note that removing get and transforming from PascalCase to snake_case is not quite sufficient.

For example, we do currently want:

-    uri     = chalkConfig.getCrashOverrideUsageReportingUrl()
+    uri     = get[string](chalkConfig, "crashoverride_usage_reporting_url")

not crash_override_usage_reporting_url. In the future, I'll suggest that we enable styleCheck and make identifiers more consistent.

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.

None yet

2 participants