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

PIPE-3873 Code architectural cleanup #884

Merged
merged 4 commits into from
Mar 30, 2023

Conversation

elliotforbes
Copy link
Contributor

@elliotforbes elliotforbes commented Mar 23, 2023

Generally improves the architectural design of the config commands to make them more easily testable.

I've extracted out the config commands into a separate package which is purely responsible for config validation and processing. It delegates all handling of various flag parsing to the cmd package which then passes them in as options.

This provides a better separation of concerns between the two packages and also means it's far easier to unit test the code and set up test cases as can be seen in the expanded unit test coverage.

There are still a few more optimisations that can be done, such as consolidating the loadYaml function into a separate package and refactoring the orb commands to use a single definition for this, but I've opted to focus purely on the config domain for now and improving that.

This takes coverage of the config package up from 0% to a far healthier:

github.com/CircleCI-Public/********-cli/config	0.258s	coverage: 63.1% of statements

Checklist

=========

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have checked for similar issues and haven't found anything relevant.
  • This is not a security issue (which should be reported here: https://circleci.com/security/)
  • I have read Contribution Guidelines.

Internal Checklist

  • I am requesting a review from my own team as well as the owning team
  • I have a plan in place for the monitoring of the changes that I am making (this can include new monitors, logs to be aware of, etc...)

Changes

=======

  • Migrates the config process and validate commands into a cleaner structure that enables us to far more easily read and test the components going forward.

Validation

=======

  • Config compiles successfully
  • Config returns appropriate error
  • Config compiles successfully with k9s host
  • Config compiles successfully with private orb resolution
  • Config compile fails when no token for private orb resolution
  • Config compile fails when no org for private orb resolution
  • Config compiles successfully with private orb and private host resolution
  • Config compile fails with private orb and private host

@elliotforbes elliotforbes requested a review from a team as a code owner March 23, 2023 09:56
@elliotforbes elliotforbes force-pushed the PIPE-2315-code-improvements branch 2 times, most recently from 55a2498 to c080e6d Compare March 23, 2023 12:14
@elliotforbes elliotforbes force-pushed the PIPE-2315-code-improvements branch 2 times, most recently from cad17cc to 547cdcc Compare March 23, 2023 17:55
@elliotforbes elliotforbes requested a review from a team as a code owner March 24, 2023 10:22
@elliotforbes elliotforbes force-pushed the PIPE-2315-code-improvements branch 2 times, most recently from 01718df to a44b456 Compare March 24, 2023 11:39
@elliotforbes elliotforbes force-pushed the PIPE-2315-code-improvements branch 3 times, most recently from 64b1475 to 47e835b Compare March 24, 2023 12:06
api/rest/client.go Outdated Show resolved Hide resolved
cmd/config.go Outdated Show resolved Hide resolved
config/commands.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
@elliotforbes elliotforbes force-pushed the PIPE-2315-code-improvements branch from 5983fbe to 39ac218 Compare March 27, 2023 07:55
@elliotforbes elliotforbes force-pushed the PIPE-2315-code-improvements branch from 0c14f3f to 670cd2d Compare March 27, 2023 08:45
Copy link
Contributor

@abdelDriowya abdelDriowya left a comment

Choose a reason for hiding this comment

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

LGTM

README.md Outdated Show resolved Hide resolved
elliotforbes and others added 2 commits March 29, 2023 09:48
Co-authored-by: Ryan William O'Hara <[email protected]>
@elliotforbes elliotforbes changed the title PIPE-2315 Code architectural cleanup PIPE-3873 Code architectural cleanup Mar 29, 2023
@elliotforbes elliotforbes merged commit f660818 into develop Mar 30, 2023
@elliotforbes elliotforbes deleted the PIPE-2315-code-improvements branch March 30, 2023 07:53
@wyardley
Copy link
Contributor

wyardley commented Mar 30, 2023

@elliotforbes are you strongly tied to the format here (with tabs and no newlines) vs. the one I introduced in #861 to resolve #860?

The changes here make the output pretty messy / hard to parse... current:

Validating config with following values
	git.base_revision:	2496ddbc7467f75ac6fd38d88396381c306f01de	id:	00000000-0000-0000-0000-000000000001	number:	1	project.git_url:	https://github.com/wyardley/circleci-cli	project.type:	github	git.tag:		git.branch:	develop	git.revision:	2496ddbc7467f75ac6fd38d88396381c306f01deNo org id or slug has been provided

Would you accept a PR to switch back to the other format (see #896)

(I also see "No org id or slug has been provided" even when no private orbs are used, and the way of specifying slugs seems to have changed; I can open separate issues for those if someone can take a look)(

wyardley added a commit to wyardley/circleci-cli that referenced this pull request Mar 30, 2023
Reintroduces the output style introduced in CircleCI-Public#861 to resolve CircleCI-Public#860, but
reverted in CircleCI-Public#884
@elliotforbes
Copy link
Contributor Author

@wyardley - no, I'm not strongly tied, let's get the improvements merged in and we can cut a new release - I thought I'd carried forward your changes but there were a lot of other moving parts in that PR.

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.

6 participants