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

More readable aws resources scan output #189

Merged
merged 4 commits into from
Feb 8, 2021
Merged

Conversation

lotoussa
Copy link
Contributor

@lotoussa lotoussa commented Feb 2, 2021

Q A
πŸ› Bug fix? no
πŸš€ New feature? no
⚠ Deprecations? no
❌ BC Break no
πŸ”— Related issues issue 79
❓ Documentation Change the doc if there's an example of scan output in it, to match with the new output.

Description

ISSUE 79: first lotoussa issue.

I first corrected the scan output of route53 resources as requested in issue 79.
Then I updated the console output (which handle aws resources output in general).
Updated the tests.

The PR is probably still in progress, because there's others aws resources containing string method that need to return either ID or adapted humanString, but i want to make a brief point of that with someone of the team.
(file concerned: aws_route_table_association_ext, aws_route_ext, aws_security_group_rule_ext)

@lotoussa lotoussa requested a review from a team as a code owner February 2, 2021 19:29
wbeuil
wbeuil previously requested changes Feb 3, 2021
Copy link
Contributor

@wbeuil wbeuil left a comment

Choose a reason for hiding this comment

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

Congrats for your first PR Louis !

As Elie told me more than once, try to keep your PR as small as possible with the issue you're working on.

As for the description of your PR, I didn't understand all of what you wrote. What is SIP ? Can you "re-read" and thus edit so that we can fully understand (or maybe I'm solo on this one and shame on me to not understand ^^).

go.sum Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_record_acceptance_test.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_record_ext.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_record_test.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_zone_ext.go Outdated Show resolved Hide resolved
@eliecharra
Copy link
Contributor

The PR title should be reworked to something clear enough to be understandable by our users (and by us) as it will be used as it in the changelog : https://github.com/cloudskiff/driftctl/releases

@eliecharra
Copy link
Contributor

New feature? | no

Actually the issue is flagged as a new feature

Copy link
Contributor

@eliecharra eliecharra left a comment

Choose a reason for hiding this comment

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

You should update stringer usage in diff output too (not only for deleted and unmanaged resources)

pkg/resource/aws/aws_route53_record_acceptance_test.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_record_test.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_record_test.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_record_test.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_record_test.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_record_test.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_zone_ext.go Outdated Show resolved Hide resolved
pkg/resource/aws/aws_route53_zone_test.go Show resolved Hide resolved
@lotoussa lotoussa added the kind/enhancement New feature or improvement label Feb 3, 2021
@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #189 (a956cf9) into main (2b51226) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   69.77%   69.79%   +0.01%     
==========================================
  Files         215      215              
  Lines        4811     4813       +2     
==========================================
+ Hits         3357     3359       +2     
  Misses       1184     1184              
  Partials      270      270              
Impacted Files Coverage Ξ”
pkg/cmd/scan/output/console.go 100.00% <100.00%> (ΓΈ)
pkg/resource/aws/aws_route53_record_ext.go 83.33% <100.00%> (+3.33%) ⬆️
pkg/resource/aws/aws_route53_zone_ext.go 100.00% <100.00%> (ΓΈ)

@lotoussa lotoussa changed the title Issues 79 lotoussa More readable aws resources scan output: Issue #79 Feb 5, 2021
@lotoussa
Copy link
Contributor Author

lotoussa commented Feb 5, 2021

I'll refresh goldenfiles another day, description PR and resolve others conversations too.

Copy link
Contributor

@eliecharra eliecharra left a comment

Choose a reason for hiding this comment

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

A way better πŸ‘πŸ» I still have small remarks

pkg/cmd/scan/output/console.go Show resolved Hide resolved
pkg/resource/aws/aws_route53_zone_test.go Outdated Show resolved Hide resolved
@eliecharra eliecharra changed the title More readable aws resources scan output: Issue #79 More readable aws resources scan output Feb 8, 2021
Copy link
Contributor

@eliecharra eliecharra left a comment

Choose a reason for hiding this comment

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

Awesome πŸ‘πŸ»

@eliecharra eliecharra dismissed wbeuil’s stale review February 8, 2021 14:35

Approved by engineering

@eliecharra eliecharra merged commit 42595c1 into main Feb 8, 2021
@eliecharra eliecharra deleted the issues_79_lotoussa branch February 8, 2021 14:39
@eliecharra eliecharra linked an issue Feb 8, 2021 that may be closed by this pull request
@wbeuil
Copy link
Contributor

wbeuil commented Feb 8, 2021

@all-contributors please add @lotoussa for code, doc

@allcontributors
Copy link
Contributor

@wbeuil

I've put up a pull request to add @lotoussa! πŸŽ‰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More readable route53 record output
4 participants