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

aws-cur: add page #6133

Merged
merged 8 commits into from Jun 23, 2021
Merged

aws-cur: add page #6133

merged 8 commits into from Jun 23, 2021

Conversation

258204
Copy link
Collaborator

@258204 258204 commented Jun 16, 2021

  • The page (if new), does not already exist in the repo.
  • The page is in the correct platform directory (common/, linux/, etc.)
  • The page has 8 or fewer examples.
  • The PR title conforms to the recommended templates.
  • The page follows the content guidelines.
  • The page description includes a link to documentation or a homepage (if applicable).

For: #5653

@258204
Copy link
Collaborator Author

258204 commented Jun 16, 2021

#5653

@bl-ue bl-ue added the new command Issues requesting creation of a new page. label Jun 17, 2021
@bl-ue bl-ue mentioned this pull request Jun 17, 2021
Copy link
Member

@mfrw mfrw left a comment

Choose a reason for hiding this comment

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

@258204 Thank you for your contribution.
I have a few review comments.

pages/common/aws-cur.md Show resolved Hide resolved
pages/common/aws-cur.md Outdated Show resolved Hide resolved
@258204
Copy link
Collaborator Author

258204 commented Jun 17, 2021

@258204 Thank you for your contribution.
I have a few review comments.

Thank you for catching those errors. The changes were applied, but something weird happened and your change requests are still listed as open.


- Create an AWS cost and usage report definition from a JSON file:

`aws cur put-report-definition --report-definition file://{{path/to/report-definition.json}}`
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use the samba URI too smb://path/to... so:

Suggested change
`aws cur put-report-definition --report-definition file://{{path/to/report-definition.json}}`
`aws cur put-report-definition --report-definition {{file://path/to/report-definition.json}}`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tried using smb on a machine with samba installed and it didn't work. Have you tried using that notation with this command? Can you share the details so I can replicate it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@navarroaxel I'm not so sure about your suggestion. If you have to use file:// every time, it's not really a variable. It's the same with quotes, where we also use "{{text}}".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bl-ue and I haven't been able to get smb://path/to... to work with aws. At this point I don't think it is compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

smb: has a different syntax anyway, smb://<host>/<path> (I believe)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, if smb:// doesn't work this is ok then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll spend some more time experimenting with smb and see if I can find a usage that will work.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it becomes too much work, forget it @285204, because it's likely that only a very few persons will even read this, let alone try it out for real.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did a little bit more and convinced myself it doesn't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a no-go then. Let's not document it.

Copy link
Member

@sbrl sbrl left a comment

Choose a reason for hiding this comment

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

Thanks, @258204!

@sbrl
Copy link
Member

sbrl commented Jun 23, 2021

Merging, since this has been open a while and @mfrw's comments appear to have been addressed.

@sbrl sbrl merged commit e55d089 into tldr-pages:main Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new command Issues requesting creation of a new page.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants