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

Updates to provide compatibility with Fog 2.4.0, take 2 #49

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

vicomte
Copy link

@vicomte vicomte commented Jun 15, 2024

Ooops, crunched my own PR #48 . Trying again.

Remaining Issues:

  • Should Aliases to the old names be included here for better compatibility?
  • LoadBalancer test failures

@vicomte vicomte marked this pull request as ready for review July 17, 2024 20:04
@vicomte
Copy link
Author

vicomte commented Jul 17, 2024

@geemus new pull req, I cancelled the prev on accident.

@geemus
Copy link
Member

geemus commented Jul 17, 2024

K , thanks. I have some travel presently, but will try to get to it week after next.

@geemus
Copy link
Member

geemus commented Aug 1, 2024

FWIW, the way I've usually done renames in stuff has been to still support the old name, but when you use it there is a deprecation warning that then passes things along to the new version. So basically aliasing but a bit noisier. Not saying we have to do that here, but thought I would share it as a data point at least.

That being said, it's a lot easier to do this with methods than classes. Not sure if there is a particularly nice/clean way to proceed or if it's more trouble than it's worth (it's also been a while since I worked on one of these conversions myself, so my memory is a little fuzzy).

@geemus
Copy link
Member

geemus commented Aug 1, 2024

I'll work on a review of what's here, but could you maybe rebase this when you get a chance? I think that should bring in the github actions/ci config so that we can get the test run and see the load balancer or other issues.

@geemus
Copy link
Member

geemus commented Aug 1, 2024

Thanks again for taking this on, I know from experience how tedious it can be to get through these renames. I was finally able to find some time to review everything and it's looking good so far.

I think it's probably good to have the aliases like you do at least for the time being for backwards compatibility, it might be slightly nicer to have those classes actually warn and then handoff like I mentioned (I guess you could do it on initialize probably). What do you think?

Hopefully if we rebase the CI will run and then that can hopefully give us something to dig into around the load balancer stuff (though it looks like master/main CI is failing already anyway, so maybe it's a broader issues anyway).

@vicomte
Copy link
Author

vicomte commented Aug 9, 2024

I'll work on a review of what's here, but could you maybe rebase this when you get a chance? I think that should bring in the github actions/ci config so that we can get the test run and see the load balancer or other issues.

This pr is current on fog-rackspace:master, not sure why it isn't kicking in.

@geemus
Copy link
Member

geemus commented Aug 9, 2024

Oops, sorry about that then. Let me try to close/reopen, sometimes that does it.

@geemus geemus closed this Aug 9, 2024
@geemus geemus reopened this Aug 9, 2024
@geemus
Copy link
Member

geemus commented Aug 13, 2024

Ok, so that does appear to have kicked off the tests again.

Unfortunately I think they are broken on main as well as this branch, which makes things harder certainly.

@vicomte Could you help point me to the particular LoadBalancer related breakage you were referring to so we can maybe try to sort that out at least?

@vicomte
Copy link
Author

vicomte commented Aug 14, 2024

The get_stats_test looks like something upstream changed what it is expecting. I looked at it briefly, but wasn't sure if changing this arg to just pass the attr hash was the correct action.

   tests/rackspace/requests/load_balancers/get_stats_tests.rb
      Fog::Rackspace::LoadBalancers | load_balancer_get_stats (rackspace)
    Initialization parameters must be an attributes hash, got Fog::Rackspace::LoadBalancers::VirtualIp     <Fog::Rackspace::LoadBalancers::VirtualIp
      id="204",
      address="246.294.440.598",
      type="PUBLIC",
      ip_version="IPV4"
    > (ArgumentError)
      /Users/mhassman/.rvm/gems/ruby-3.2.2/gems/fog-core-2.4.0/lib/fog/core/collection.rb:89:in `new'
      /Users/mhassman/.rvm/gems/ruby-3.2.2/gems/fog-core-2.4.0/lib/fog/core/collection.rb:79:in `block in load'
      /Users/mhassman/.rvm/gems/ruby-3.2.2/gems/fog-core-2.4.0/lib/fog/core/collection.rb:19:in `each'
      /Users/mhassman/.rvm/gems/ruby-3.2.2/gems/fog-core-2.4.0/lib/fog/core/collection.rb:19:in `each'
      /Users/mhassman/.rvm/gems/ruby-3.2.2/gems/fog-core-2.4.0/lib/fog/core/collection.rb:79:in `load'
      /Users/mhassman/Projects/fog-rackspace/lib/fog/rackspace/models/load_balancers/load_balancer.rb:111:in `virtual_ips='

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.

4 participants