-
-
Notifications
You must be signed in to change notification settings - Fork 119
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
adding support for --includeTags option #108
Conversation
Thanks for opening this pull request! |
Thanks for the PR, I will have a look and get back to you! |
Hello Michiel.
I have added token management for accounts that need autentication for
non-default tenants, and for service principal management.
Would you like another PR, or consolidate both PRs?
El mié, 14 feb 2024, 7:49 p. m., Michiel van Oudheusden <
***@***.***> escribió:
… Thanks for the PR, I will have a look and get back to you!
—
Reply to this email directly, view it on GitHub
<#108 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABW74J45T3IAHIIDQZ5SO6DYTUBMHAVCNFSM6AAAAABDIVVFBOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBUGQYDIOJVHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Oh nice, preferably another PR, please. |
Ok, I will wait for your acceptance, and then I will file the new PR. I dont know if can be handle separatelly in git. Thanks. |
@@ -210,7 +210,7 @@ private async Task<HttpResponseMessage> ExecuteCallToCostApi(bool includeDebugOu | |||
|
|||
var currency = row[3].ToString(); | |||
|
|||
var costItem = new CostItem(date, value, valueUsd, currency); | |||
var costItem = new CostItem(date, value, valueUsd, currency, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You set the currency to an empty string here. A specific reason for that? This should represent the currency the billing is in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Last argument is the Tag. Reason to be empty is because in that method includeTags do not apply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, misreading it here.
Congrats on merging your first pull request! |
Looks good, I made some small changes in the tag parsing and cvs formatter. Otherwise the json output will show tags as strings, I converted those to dictionary of string, string. But then I needed a csvformatter to get it in a string again for the CSV output. So thanks for the PR! I will publish a package asap. |
Thanks. I will Check it tomorrow and will feed you back. I think Json was
converted because on null scenarios "ugly" [] was the output. Maybe in the
methods not related to the DailyCost.
El vie, 16 feb 2024, 10:45 p. m., Michiel van Oudheusden <
***@***.***> escribió:
… Looks good, I made some small changes in the tag parsing and cvs
formatter. Otherwise the json output will show tags as strings, I converted
those to dictionary of string, string. But then I needed a csvformatter to
get it in a string again for the CSV output.
So thanks for the PR! I will publish a package asap.
—
Reply to this email directly, view it on GitHub
<#108 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABW74J7YMTJPDAM6FESZKHDYT7HOTAVCNFSM6AAAAABDIVVFBOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBZGM3TONJXGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
The package is ready, so have a go with it and see if this suits your needs. |
the PR I summitted handled the Tags as Json where empty strings were [] |
Is that in the CSV output? |
in Json:
in csv: Date,Name,Cost,CostUsd,Currency,Tags |
Hmm, well the Tags are structured as a dictionary, which translates to an object in JSON, so this is an empty object Are you unable to parse it now at your end? |
I see the changes you added. You convert to key-value dictionary here:
I prefered as Json, that is easier to handle in Power Query transformations as Json data type:
maybe key-value works fine too in Power Query. you should change the readme anyway, because I said Tags were Json format. |
Ah got you. Hmm tricky one as it is a CSV output, not a JSON output. Can you consume the JSON output instead? Or use a function to convert the string?
Or what would be a better format to have it in? I would even think that with a CSV output, I would need a column per tag key. So in your example:
Would that work? |
If parsing the text to json in M is easy as you show, it is fine to me. |
I m no power query expert, so this is untested. I m just hesitant to introduce a JSON object in CSV output, as we already have dedicated JSON output. I have some logic in the |
I would not add new columns from the Tags because tagging follows customer naming conventions, gobernance, etc. etc. |
They will be dynamic in nature, but it can easily add a lot of additional columns if various different tags are being used. You will get an excellent overview, though, if you have a couple of tags. So, like owner, business unit, cost center, etc, there will be extra columns, and you can easily consume those in Excel and sort/filter on without splitting. Then it is still up to the ETL what it pulls into the model, but it might be harder to iterate through those dynamic columns to reconstruct the key/value combinations. Cons and pros; let me know if you can work with the current solution, if not, we come up with something. |
If you can expand the tags names in columns would be great :) I'd add:
I'd include the Tag as the last column in case someone wants to extend in Power Query. BTW, It would be nice to have, not mandatory. I am happy how it is now! |
Then you get this: #111 |
Yes. Exactly! In fact, it may be intereseting to the users adding an option --expandColumns "col1,col2,col3" to be specific in what columns to expand. If the columns list is not present, expand with Microsoft tagging said earlier. but seems a bit complex... I am not sure if I am suggesting too much, and at the end of the day, nobody uses it :) It may be interesting a pool where people vote to! |
In my mind, this maps now nicely to CSV. So when you include the tags, you get them as additional columns in CSV output, while in JSON you get them as a dictionary. I have used this way of working before, so it also keeps it consistent. So I will merge the PR, but do let me know if you can continue to work, as you initiated this PR/discussion for a reason. |
it is good how it is now. |
Hello @mivano
I need to export the daily costs data by resourceId and include into the resultset the resource Tags.
It is useful for scenarios where data is exported to analyze using Power BI.
I have changed the code to support exporting Tags when:
I have chaged the API version to 2023-03-01.
This is the logic of the new option:
I had to add a new record called CostDailyItemWithoutTags where rows were moved when includeTags is False.
It was a small switch in the Csv Output formaters depending on the setgings:
For the Json formater was easier:
I have edited the readme.md to include usage description.
I am happy you consider adding this option to the main brach!
Regards,
Eladio