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

CFINSPEC-246/CFINSPEC-247 Attestation changes for N/R outcomes #6222

Merged
merged 26 commits into from
Sep 30, 2022

Conversation

Nik08
Copy link
Contributor

@Nik08 Nik08 commented Aug 2, 2022

Description

Attestation feature which enables attesting N/R controls and marking them pass or fail manually using a file option --attestation-file

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New content (non-breaking change)
  • Breaking change (a content change which would break existing functionality or processes)

Checklist:

  • I have read the CONTRIBUTING document.

Nik08 added 16 commits August 2, 2022 15:01
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
… the control - fix in streaming reporter

Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
…iles

Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
…tions file

Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
…aming reporter

Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
@Nik08 Nik08 requested review from a team as code owners August 2, 2022 09:36
@netlify
Copy link

netlify bot commented Aug 2, 2022

Deploy Preview for chef-inspec canceled.

Name Link
🔨 Latest commit a3b358e
🔍 Latest deploy log https://app.netlify.com/sites/chef-inspec/deploys/62f265e00293420008f49c39

@github-actions github-actions bot added the Documentation ZH multi-repo label for the docs-team label Aug 2, 2022
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
…station file

Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
# logic to find expiration date using frequency and updated date.
updated_last = attestation_data["updated"]
begin
if updated_last.is_a?(Date) || (updated_last.is_a?(String) && Date.parse(updated_last).year != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can write this whole bunch of if and else in another method to make it cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let me think about how I can logically separate those lines since I think they are part of the logic used for fetching the expiration date.

Signed-off-by: Deepa Kumaraswamy <dkumaras@progress.com>
ui.exit(:usage_error)
end
end
end

def self.fetch_expiry_using_frequency_and_updated_date(updated_date, frequency, control_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this method name can be reduced to fetch_expiry_date as its arguments are self-explanatory in that they need, updated_date and frequency. You can add code comments.

Copy link
Contributor Author

@Nik08 Nik08 Aug 3, 2022

Choose a reason for hiding this comment

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

I think the method name should also explain its usage. So it seems fine to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem I felt it's too long :)

Signed-off-by: Deepa Kumaraswamy <dkumaras@progress.com>
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Reviewed and discussed in meetings - thanks for the good work!

Copy link
Contributor

@Vasu1105 Vasu1105 left a comment

Choose a reason for hiding this comment

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

Other than the long method name all looks good. Thanks!!

Nik08 and others added 3 commits August 8, 2022 13:52
…ation using frequency and updated date

Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Deepa Kumaraswamy <dkumaras@progress.com>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
Signed-off-by: Nikita Mathur <nikita.mathur@chef.io>
@clintoncwolfe clintoncwolfe changed the base branch from main to inspec-6 September 29, 2022 13:43
@clintoncwolfe clintoncwolfe merged commit efc6f2c into inspec-6 Sep 30, 2022
@clintoncwolfe clintoncwolfe deleted the nm/attestations-feature branch September 30, 2022 13:53
@clintoncwolfe clintoncwolfe restored the nm/attestations-feature branch August 25, 2023 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation ZH multi-repo label for the docs-team Expeditor: Bump Minor Version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants