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

Support azure migrate assessments #424

Merged
merged 31 commits into from
Sep 24, 2021

Conversation

sathish-progress
Copy link
Contributor

@sathish-progress sathish-progress commented Aug 4, 2021

Description

Support singular and plural resource for Azure Migrate Assessment(s)

Also fixed #425 as part of this

Issues Resolved

#425

Check List

Signed-off-by: Sathish <sbabu@progress.com>
Signed-off-by: Sathish <sbabu@progress.com>
Signed-off-by: Sathish <sbabu@progress.com>
Signed-off-by: Sathish <sbabu@progress.com>
Signed-off-by: Sathish <sbabu@progress.com>
Signed-off-by: Sathish <sbabu@progress.com>
@sathish-progress sathish-progress requested a review from a team as a code owner August 4, 2021 05:51
@sathish-progress sathish-progress linked an issue Aug 4, 2021 that may be closed by this pull request
Copy link
Contributor

@sa-progress sa-progress left a comment

Choose a reason for hiding this comment

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

azureStorageRedundancy has other 4 types : GeoRedundant, ReadAccessGeoRedundant, Unknown, ZoneRedundant.
Shouldn't we provide this value as default value in variable.tf and other values like scalingFactor as well?

@sathish-progress
Copy link
Contributor Author

azureStorageRedundancy has other 4 types : GeoRedundant, ReadAccessGeoRedundant, Unknown, ZoneRedundant.
Shouldn't we provide this value as default value in variable.tf and other values like scalingFactor as well?

sorry I didn't understand, this PR is about Azure Migrate Assessments and not about Azure Storage Redundancy ..

@sathish-progress sathish-progress added Documentation Version: Bump Minor Used by github.minor_bump_labels to bump the Minor version number. labels Aug 5, 2021
Copy link
Contributor

@sa-progress sa-progress left a comment

Choose a reason for hiding this comment

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

There are minor comments please check.


| Name | Description |
|----------------|----------------------------------------------------------------------------------|
| name | Name of the Azure Migrate Assessments to test. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Name is twice .Please check once .

opts[:resource_path] = [opts[:project_name], 'groups', opts[:group_name], 'assessments'].join('/')
super(opts, true)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

to_s implementation is missing .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@table << resource.merge(resource[:properties])
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

to_s implementation is missing .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Sathish <sbabu@progress.com>
Signed-off-by: Sathish <sbabu@progress.com>
Signed-off-by: Sathish <sbabu@progress.com>
Signed-off-by: Sathish <sbabu@progress.com>
@sathish-progress sathish-progress requested a review from a team as a code owner August 19, 2021 08:16
Copy link
Contributor

@sa-progress sa-progress left a comment

Choose a reason for hiding this comment

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

LGTM

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

@IanMadd IanMadd left a comment

Choose a reason for hiding this comment

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

Feedback for @dkumaras


## Syntax

`name` is a required parameter, and `resource_group` is an optional parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 52 shows four required parameters, not two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is done

`name` is a required parameter, and `resource_group` is an optional parameter.

```ruby
describe azure_migrate_assessment(resource_group: 'MIGRATED_VMS', project_name: 'ZONEA_MIGRATE_ASSESSMENT_PROJECT', group_name: 'ZONEA_MACHINES_GROUP', NAME: 'ZONEA_MACHINES_MIGRATE_ASSESSMENT') do
Copy link
Contributor

Choose a reason for hiding this comment

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

The placeholder values for the parameters should be more generic. So MIGRATED_VMS can just be RESOURCE_GROUP, ZONEA_MIGRATE_ASSESSMENT_PROJECT can just be PROJECT_NAME, ZONEA_MACHINES_GROUP can just be GROUP_NAME, and ZONEA_MACHINES_MIGRATE_ASSESSMENT should be MIGRATE_ASSESSMENT_NAME.

The same goes for the other code examples.


# azure_migrate_assessment

Use the `azure_migrate_assessment` InSpec audit resource to test the properties related to Azure Migrate Assessments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Assessment is a generic term so it should be lowercase. The same goes for all the other uses of "assessment". It's also singular "assessment" in this resource, but plural in the other resource.


The parameter set should be provided for a valid query:

- `resource_group` and `project_name` and `group_name` and `name`
Copy link
Contributor

Choose a reason for hiding this comment

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

The ands can be replaced with commas.

@@ -31,7 +31,7 @@ where

The following parameters can be passed for targeting a specific Azure resource.

| Name | Description |
| Name |Description |
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this change.

docs/resources/azure_migrate_assessment.md Outdated Show resolved Hide resolved
docs/resources/azure_migrate_assessment.md Outdated Show resolved Hide resolved
docs/resources/azure_migrate_assessments.md Outdated Show resolved Hide resolved
docs/resources/azure_migrate_assessments.md Outdated Show resolved Hide resolved
sathish-progress and others added 4 commits September 13, 2021 11:25
Signed-off-by: Sathish <sbabu@progress.com>
Co-authored-by: Ian Maddaus <IanMadd@users.noreply.github.com>
Co-authored-by: Ian Maddaus <IanMadd@users.noreply.github.com>
Co-authored-by: Ian Maddaus <IanMadd@users.noreply.github.com>
sathish-progress and others added 5 commits September 13, 2021 11:27
Co-authored-by: Ian Maddaus <IanMadd@users.noreply.github.com>
Signed-off-by: Sathish <sbabu@progress.com>
@sathish-progress
Copy link
Contributor Author

@dkumaras , Could you please amend your signature to your commits that is causing the DCO to fail?

Signed-off-by: Deepa Kumaraswamy <dkumaras@progress.com>
Signed-off-by: Deepa Kumaraswamy <dkumaras@progress.com>
…e.md

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

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sathish-progress sathish-progress merged commit b6dbffa into main Sep 24, 2021
@sathish-progress sathish-progress deleted the support-azure-migrate-assessments branch September 24, 2021 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Version: Bump Minor Used by github.minor_bump_labels to bump the Minor version number.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dynamic resource methods with float values are being ignored
4 participants