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

ADD dax module #13

Merged
merged 7 commits into from
Dec 14, 2021
Merged

ADD dax module #13

merged 7 commits into from
Dec 14, 2021

Conversation

fastboot
Copy link
Contributor

@fastboot fastboot commented Dec 6, 2021

Description

Description of what this PR does. What have you added or changed, and why? If it fixes a bug or resolves a feature request, be sure to link to that issue.

This PR adds a dax module to the already present terraform modules.

Review Checks

Please check if the PR fulfills these requirements:

Put an x in the boxes that apply, Remove any lines that do not apply

  • 📝 The commit message is clear and descriptive
  • 🔐 The Security Considerations section in the PR description is complete - Please do not remove this
  • ✅ Tests for the changes have been added and run successfully including the new changes
  • 📄 Documentation has been added / updated (for bug fixes / features)

Dependencies

Add links to any pull requests or documentation related to this pull request.

None

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Yes
  • No breaking changes

Security Considerations

Are there any other security or data concerns to be aware of?

Please discuss the security implications/considerations relevant to the proposed change.
This may include...

  • security-relevant design decisions
  • concerns
  • important discussions
  • implementation-specific guidance and pitfalls
  • an outline of threats and risks and how they are being addressed.

All security considerations in place.

Types of change

What kind of change does this Pull Request introduce?
Put an x in the boxes that apply

  • 🐛 Bugfix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 🛠 Adding or updating configuration files, development scripts etc.
  • ♻️ Refactoring (no functional changes, no api changes)
  • 🧹 Chore (removing redunant files, fixing typos etc.)
  • 📄 Documentation Update
  • ❓ Other (if none of the other choices apply)

Testing

Please include steps that the reviewer can follow in order to test the changes

  1. Try to import and use dax module as explained in Readme for the repository.

dax/README.md Outdated Show resolved Hide resolved
dax/main.tf Outdated Show resolved Hide resolved
dax/outputs.tf Outdated Show resolved Hide resolved
dax/variables.tf Outdated Show resolved Hide resolved
Copy link
Member

@azhar22k azhar22k left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just some comments about it being generic

dax/README.md Show resolved Hide resolved
Copy link

@shreyance-jain shreyance-jain left a comment

Choose a reason for hiding this comment

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

looks fine, check for the other suggested changes.

dax/README.md Outdated Show resolved Hide resolved
Copy link

@shubhamPeak shubhamPeak left a comment

Choose a reason for hiding this comment

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

looks good to me, just few readme file updates

Copy link
Member

@azhar22k azhar22k left a comment

Choose a reason for hiding this comment

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

Some good comments from @shubhamPeak . rest looks good

dax/README.md Outdated Show resolved Hide resolved
@azhar22k azhar22k merged commit d1e3d28 into peak-ai:master Dec 14, 2021
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.

4 participants