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.

Sign up for GitHub

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

Jump to bottom

Missing no-refresh for exos_facts in Interfaces #517

Merged
merged 2 commits into from
Feb 13, 2023
Merged

Missing no-refresh for exos_facts in Interfaces #517

merged 2 commits into from
Feb 13, 2023

Conversation

plazukin
Copy link
Contributor

@plazukin plazukin commented Feb 1, 2023 •
edited
Loading

SUMMARY

Without the change exos_facts return timeout error when we use connection type network_cli.
It happened because we send command without 'no-refresh' and script cli2json.py stuck in loop while reading console output

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

exos: exos_facts

ADDITIONAL INFORMATION

You can reproduction of the problem with this task and 'ansible_connection: network_cli':

    - name: Gather all facts
      exos_facts:
        gather_subset:
          - config
          - interfaces

I use the component with EXOS 16.2.5.4, but also face a problem in EXOS 30.5.1.15

@plazukin
Copy link
Contributor Author

plazukin commented Feb 1, 2023

@Andersson007, hi!
Сould you see my pull request?

@Andersson007
Copy link
Contributor

@plazukin hi, thanks for the PR!

  • we should ensure this change is not breaking (for older versions of the underlying technology)
  • could you please add a changelog fragment ?

cc @LindsayHill @ujwalkomarla @hlrichardson @rdvencioneck @JayalakshmiV @JayalakshmiV you are mentioned as maintainers of exos_ related stuff, so could you please take a look?

@plazukin
Copy link
Contributor Author

plazukin commented Feb 2, 2023

@plazukin hi, thanks for the PR!

  • we should ensure this change is not breaking (for older versions of the underlying technology)
  • could you please add a changelog fragment ?

cc @LindsayHill @ujwalkomarla @hlrichardson @rdvencioneck @JayalakshmiV @JayalakshmiV you are mentioned as maintainers of exos_ related stuff, so could you please take a look?

Changelog fragment added!
How can i help you with check the fix?

Andersson007
Andersson007 reviewed Feb 2, 2023
View reviewed changes
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@plazukin thanks for adding the fragment!
There's a small formatting thing to change.

As there are no integration tests, we cannot test it here.
Just be sure the no-refresh is supported in older versions of the service. Please check their docs to check.

Not being a network engineer, I provide only general support for the collection like releasing, CI, etc.
Let's wait for someones feedback until next week. If there's no feedback, I'll merge the PR after Fosdem, i.e. at the end of the next week.

Thank you

changelogs/fragments/517-add-ports-no-refresh-in-exos-facts.yaml Outdated
Comment on lines 2 to 3
- exos_facts return timeout error when we use connection type network_cli.
It happened because we send command without 'no-refresh' and script cli2json.py stuck in loop while reading console output
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- exos_facts return timeout error when we use connection type network_cli.
It happened because we send command without 'no-refresh' and script cli2json.py stuck in loop while reading console output
- exos_facts - returns timeout error when we use connection type ``network_cli``.
It happened because we send command without ``no-refresh`` and script ``cli2json.py`` stuck in loop while reading console output (https://github.com/ansible-collections/community.network/pull/517).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it. Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, thanks, so let's wait until the mid/end of the next week, then I'll merge if no objections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, i hope that all fine.
Thank you :)

@plazukin
Copy link
Contributor Author

@Andersson007, hi!
It's time to merge it!
Can we do this? :)

@Andersson007 Andersson007 merged commit ffdd7c2 into ansible-collections:main Feb 13, 2023
@Andersson007
Copy link
Contributor

@plazukin hi, thanks for the reminder! busy time. Please do it in future PRs:)
Feel free also to add yourself as the module's maintainer to .github/BOTMETA.yml for the file or group of files.
The bot doesn't work at the moment but in future you'll be notified if someone reports, say, a bug:)
Also we'd be happy to see more contributions from you here and across other collections. I myself a maintainer of community.mysql and community.postrgresql (and i can help with others).
Here's also the contributor path which can guide you.
Any questions? Feel free to ask (join Matrix channels, at lease community and social).
Thanks for the contribution!

@Andersson007 Andersson007 added backport-4 backport-5 Automatically create a backport for the stable-5 branch labels Feb 13, 2023
@patchback
Copy link

patchback bot commented Feb 13, 2023 •
edited
Loading

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/ffdd7c2844c0e192d77653a95bcf5094d08fbf74/pr-517

Backported as #518

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Feb 13, 2023
@plazukin @patchback
Missing no-refresh for exos_facts in Interfaces (#517)
d074725 
* missing no-refresh for exos_facts in Interfaces

* exos_fixtures: rename show_port_config

---------

Co-authored-by: Pavel Lazukin <plazukin@croc.ru>
(cherry picked from commit ffdd7c2)
@patchback patchback bot mentioned this pull request Feb 13, 2023
Merged
@ansible-collections ansible-collections deleted a comment from patchback bot Feb 13, 2023
@Andersson007
Copy link
Contributor

@plazukin could you please manually backport the changes to stable-4? It cannot be automatically backported by our bot, i guess, because the file lived in another path.

Andersson007 pushed a commit that referenced this pull request Feb 13, 2023
@patchback @plazukin
Missing no-refresh for exos_facts in Interfaces (#517) (#518)
1fe1e7a 
* missing no-refresh for exos_facts in Interfaces

* exos_fixtures: rename show_port_config

---------

Co-authored-by: Pavel Lazukin <plazukin@croc.ru>
(cherry picked from commit ffdd7c2)

Co-authored-by: Pavel Lazukin <45261138+plazukin@users.noreply.github.com>
@Andersson007
Copy link
Contributor

@plazukin could you please take a look ^?

@plazukin
Copy link
Contributor Author

@plazukin could you please take a look ^?

@Andersson007, sorry i was on vacation
Found new commits from the community. What do I need to do now? Do I need to make the backport stable-4?

@Andersson007
Copy link
Contributor

@plazukin could you please take a look ^?

@Andersson007, sorry i was on vacation Found new commits from the community. What do I need to do now? Do I need to make the backport stable-4?

@plazukin yep, you can use https://docs.ansible.com/ansible/devel/community/development_process.html#backporting-merged-prs-in-ansible-core (use main instead of devel as the manual is for ansible core)

plazukin added a commit to plazukin/community.network that referenced this pull request Feb 27, 2023
@plazukin
Missing no-refresh for exos_facts in Interfaces (ansible-collections#517
2139af5 
)
@plazukin plazukin mentioned this pull request Feb 27, 2023
Merged
Andersson007 pushed a commit that referenced this pull request Feb 27, 2023
@plazukin
Missing no-refresh for exos_facts in Interfaces (#517) (#526)
de71e41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andersson007 Andersson007 Andersson007 approved these changes

Assignees
No one assigned
Labels
backport-5 Automatically create a backport for the stable-5 branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@plazukin @Andersson007