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

Asset cleanup #240

Merged
merged 7 commits into from
Feb 14, 2020
Merged

Asset cleanup #240

merged 7 commits into from
Feb 14, 2020

Conversation

nicpottier
Copy link
Member

Bit of refactoring around how our assets are loaded etc..

Callers can now request a cached org asset but can specify which pieces of the cache to refresh. This comes in handy for those endpoints that absolutely must have the most current stuff.

Also need some more thought around mucking with flows since new contact action endpoint needs to run "fake" flows in production. Decided to add a Clone method that builds new assets but doesn't cache the result (and bases it off an existing env). Some runtime panics make sure you don't try to modify uncloned assets.

return nil, errors.Wrapf(err, "error marshalling flow definition")
func (a *OrgAssets) SetFlow(id FlowID, uuid assets.FlowUUID, name string, definition json.RawMessage) *Flow {
if !a.cloned {
panic("can only override flow definitions on cloned orgs")
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't necessarily love this approach.. the better one would be to have a different type of org that satisfied the org interface but had this way of being modified, but that would have exceeded my budget for refactoring for the day and would require a lot of copy/paste.

@codecov
Copy link

codecov bot commented Feb 14, 2020

Codecov Report

Merging #240 into master will decrease coverage by 0.40%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
- Coverage   48.94%   48.54%   -0.41%     
==========================================
  Files          82       83       +1     
  Lines        7398     7457      +59     
==========================================
- Hits         3621     3620       -1     
- Misses       3205     3273      +68     
+ Partials      572      564       -8     
Impacted Files Coverage Δ
web/testing.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2069bcc...0faa929. Read the comment docs.

@@ -68,11 +68,6 @@ func (f *Flow) FlowType() FlowType { return f.f.FlowType }
// Version returns the version this flow was authored in
func (f *Flow) Version() string { return f.f.Version }

// SetDefinition sets our definition from the passed in definition
Copy link
Member Author

Choose a reason for hiding this comment

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

We want all our assets to be immutable. This is now handled by calling SetFlow on OrgAssets.

// our flow contact
contact, err := c.FlowContact(org, sa)
contact, err := c.FlowContact(org)
Copy link
Member Author

Choose a reason for hiding this comment

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

A lot of signature changes like this as session assets are now cached on the org which makes the calls a lot simpler in many places. SessionAssets are thread safe and immutable ya @rowanseymour ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the only cleverness is trying to cache flow defs

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the caching thread safe?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicpottier nicpottier merged commit bd63c65 into master Feb 14, 2020
@nicpottier nicpottier deleted the asset-cleanup branch February 14, 2020 19:18
rasoro pushed a commit to Ilhasoft/mailroom that referenced this pull request Dec 16, 2024
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.

2 participants