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

Upgrade to avo 3 #4342

Merged
merged 27 commits into from
Oct 14, 2024
Merged

Upgrade to avo 3 #4342

merged 27 commits into from
Oct 14, 2024

Conversation

segiddins
Copy link
Member

No description provided.

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Attention: Patch coverage is 89.59459% with 77 lines in your changes missing coverage. Please review.

Project coverage is 96.02%. Comparing base (8021e2e) to head (56325b1).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
app/avo/resources/api_key.rb 19.35% 25 Missing ⚠️
app/avo/resources/ip_address.rb 38.46% 8 Missing ⚠️
app/avo/resources/geoip_info.rb 30.00% 7 Missing ⚠️
app/avo/resources/oidc_api_key_role.rb 70.83% 7 Missing ⚠️
app/avo/resources/dependency.rb 33.33% 6 Missing ⚠️
app/avo/resources/gem_typo_exception.rb 54.54% 5 Missing ⚠️
app/avo/resources/organization.rb 61.53% 5 Missing ⚠️
app/avo/resources/membership.rb 55.55% 4 Missing ⚠️
app/avo/resources/api_key_rubygem_scope.rb 57.14% 3 Missing ⚠️
app/avo/actions/application_action.rb 90.47% 2 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4342      +/-   ##
==========================================
- Coverage   96.97%   96.02%   -0.95%     
==========================================
  Files         424      424              
  Lines        8892     8942      +50     
==========================================
- Hits         8623     8587      -36     
- Misses        269      355      +86     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

help: "A comment explaining why this action was taken.<br>Will be saved in the audit log.<br>Must be more than 10 characters."
end

# Would be nice if there was a way to force a field to show up as visible
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @adrianthedev this seems to be something where avo3 is stricter than avo2, and it would be nice if there was a way to force a field to be visible even if avoiding thinks it shouldn't be

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @segiddins. Can you please share what is the problem? Maybe I can figure out an easier fix for your use case and remove this monkeypatch.

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 have fields that we need to show up in forms, even though Avo considers them "computed" (because the underlying record has no setter for them)

@segiddins segiddins force-pushed the segiddins/avo-3 branch 3 times, most recently from bd5bf10 to 7e74bc8 Compare September 23, 2024 18:51
@segiddins segiddins marked this pull request as ready for review September 23, 2024 22:03
@segiddins segiddins changed the title [WIP] Upgrade to avo 3 Upgrade to avo 3 Sep 23, 2024
@segiddins
Copy link
Member Author

@adrianthedev @simi this should be ready to review! Only thing is it would be nice to point to a release of avo that removes the activestorage dependency instead of pointing to a git commit

@segiddins
Copy link
Member Author

This is green and ready to ship, @simi please review

@simi
Copy link
Member

simi commented Oct 2, 2024

👀

@adrianthedev
Copy link
Contributor

@adrianthedev @simi this should be ready to review! Only thing is it would be nice to point to a release of avo that removes the activestorage dependency instead of pointing to a git commit

The latest release should have the AS dependency removed.

In the next release we will remove literal types altogether so we'll have less problems like those.

Copy link
Member

@indirect indirect 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, a couple minor things that seem like they need to be cleaned up. Thanks for making this happen!

Gemfile Outdated
@@ -62,12 +62,20 @@ gem "faraday-multipart", "~> 1.0"
gem "timescaledb", "~> 0.3"

# Admin dashboard
gem "avo", "~> 2.53"
# github needed until https://github.com/avo-hq/avo/pull/3215 is released
Copy link
Member

Choose a reason for hiding this comment

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

it was released 😅

Rakefile Outdated
# TODO: remove this when we point back to a release version of Avo
namespace :assets do
task precompile: "avo:build-assets"
end
Copy link
Member

Choose a reason for hiding this comment

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

this PR is back on an Avo release now right?

# if avo_request_pattern.matches?(request)
# { status: 200, body: { id: :pro, valid: true, payload: {} }.to_json,
# headers: { "Content-Type" => "application/json" } }
# end
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the comment? I’d rather remove things that don’t run if we can

@adrianthedev
Copy link
Contributor

I still have eyes on this PR, so feel free to ping me if I can help.

@segiddins
Copy link
Member Author

@simi @colby-swandale just waiting on y'all to review to ship this, it needs a rebase almost every time we update deps

# allow dotenv to specify RAILS_GROUPS
if defined?(Dotenv::Rails)
Dotenv::Rails.load
Bundler.require(*Rails.groups)
Copy link
Member

Choose a reason for hiding this comment

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

Any chance we can move the duplicate Bundler.require or is this required for Dotenv?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's required to allow .Env to control the rails groups

@colby-swandale
Copy link
Member

@segiddins apologies, I've been a bit under the weather the past few days. I merged #4883 today, so you will need to rebase once again, apologies 🙇🏻

Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
@segiddins segiddins enabled auto-merge (squash) October 14, 2024 21:24
@segiddins segiddins merged commit 4b369c0 into master Oct 14, 2024
17 of 19 checks passed
@segiddins segiddins deleted the segiddins/avo-3 branch October 14, 2024 21:30
@adrianthedev
Copy link
Contributor

Awesome @segiddins!
Thanks for putting in the work to make this upgrade happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants