Skip to content

[usage] Add usageAttributionID to WorkspaceInstance model (in go) #10927

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

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Jun 27, 2022

Description

Adds model definition to consume usage attribution id

Related Issue(s)

How to test

Unit tests

Release Notes

NONE

Werft options:

  • /werft with-preview

@easyCZ easyCZ force-pushed the jx/billing-attribution branch from fa4ad76 to 1320d58 Compare June 27, 2022 09:19
@easyCZ easyCZ force-pushed the mp/usage-attribution-id branch from c414011 to 34d0776 Compare June 27, 2022 09:21
@gitpod-io gitpod-io deleted a comment from github-actions bot Jun 27, 2022
@roboquat roboquat added size/S and removed size/XXL labels Jun 27, 2022
@easyCZ easyCZ force-pushed the mp/usage-attribution-id branch from 223c661 to 2a54e55 Compare June 27, 2022 11:48
@jankeromnes jankeromnes force-pushed the jx/billing-attribution branch from 1320d58 to 898d9ed Compare June 27, 2022 14:00
Base automatically changed from jx/billing-attribution to main June 27, 2022 14:11
@roboquat roboquat added size/L and removed size/S labels Jun 27, 2022
@easyCZ easyCZ force-pushed the mp/usage-attribution-id branch from 2a54e55 to b966efd Compare June 27, 2022 15:17
@roboquat roboquat removed the size/L label Jun 27, 2022
@easyCZ easyCZ marked this pull request as ready for review June 27, 2022 15:17
@easyCZ easyCZ requested a review from a team June 27, 2022 15:17
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Jun 27, 2022
@easyCZ easyCZ force-pushed the mp/usage-attribution-id branch from b966efd to d416916 Compare June 28, 2022 08:08
@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 28, 2022

Many thanks for this PR!

In general, I'm very much in favor of multiple smaller PRs. However (and maybe this is just me adjusting to this new paradigm), I still struggle with reviewing code that isn't used in any meaningful way.

Thus, I took a peek at your other PR: #10938 👀

  • There, I saw how the attribution.Values() function is being used 👍 (helps me build confidence that it's indeed what we want)
  • However, I also saw there that the NewAttributionID(entity string, identifier string) is not the most useful method, proven by the fact that you've refactored it into db.NewUserAttributionID(identifier string) and db.NewTeamAttributionID(identifier string) -- would it make sense to hoist the better implementation in this PR?

@easyCZ
Copy link
Member Author

easyCZ commented Jun 28, 2022

I still struggle with reviewing code that isn't used in any meaningful way.

Here, the constructor and the destructor are mostly for completness and symmetry. Agree that ideally the destructor wasn't used in this PR (and perhaps should've been used in tests) but it felt incomplete without it.

However, I also saw there that the NewAttributionID(entity string, identifier string) is not the most useful method, proven by the fact that you've refactored it into db.NewUserAttributionID(identifier string) and db.NewTeamAttributionID(identifier string) -- would it make sense to hoist the better implementation in this PR?

This would also suffer from the same problem as above. I'd be creating a constructor for NewTeamAttributionID without having usage for it in this PR, so that felt even more of an over-kill to include in this one.
On the other hand, once I had practice usage, it longer made sense to keep the NewAttributionID function public so it got refectored. This mostly shows that you raise PRs as you go, rather than figuring out what the perfect final version is (and back-slicing it then).

@easyCZ easyCZ force-pushed the mp/usage-attribution-id branch from d416916 to c48fee9 Compare June 28, 2022 09:42
@roboquat roboquat removed the size/S label Jun 28, 2022
@easyCZ
Copy link
Member Author

easyCZ commented Jun 28, 2022

I've pulled in the two methods and added some tests for using the Value to showcase it better. Hopefully this addresses the above comment.

Copy link
Contributor

@jankeromnes jankeromnes 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, many thanks! 🚀

@roboquat roboquat merged commit b0170ec into main Jun 28, 2022
@roboquat roboquat deleted the mp/usage-attribution-id branch June 28, 2022 09:47
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants