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

added support for appmesh virtualnode #9378

Merged
merged 6 commits into from
Mar 29, 2024
Merged

Conversation

Johnlon
Copy link
Contributor

@Johnlon Johnlon commented Mar 23, 2024

Need help with the "doc" step - I renamed something and the doc step is failing.
Not sure how to fix.
Thanks

@Johnlon Johnlon requested a review from kapilt as a code owner March 23, 2024 03:34
@Johnlon
Copy link
Contributor Author

Johnlon commented Mar 27, 2024

@ajkerrigan Hi sorry to lean directly on you on this but I want to get this addition in before the other PR makes it into a build.
I renamed something so make it consistent with this new feature.

I don't want a release to go out with the old value of virtual-gateway as then I end up break backward compat.

The doco is breaking because I renamed "virtual-gateway" to "virtualgateway".

Other than that then this new addition is basically a clone of the existing virtualgateway resource and tests.

Thanks in advance.

c7n/resources/appmesh.py Outdated Show resolved Hide resolved
c7n/resources/resource_map.py Show resolved Hide resolved
@ajkerrigan
Copy link
Member

@ajkerrigan Hi sorry to lean directly on you on this but I want to get this addition in before the other PR makes it into a build. I renamed something so make it consistent with this new feature.

I don't want a release to go out with the old value of virtual-gateway as then I end up break backward compat.

The doco is breaking because I renamed "virtual-gateway" to "virtualgateway".

Other than that then this new addition is basically a clone of the existing virtualgateway resource and tests.

Thanks in advance.

Suggestions inline. Appreciate the thoughts about consistency. Thank you.

@Johnlon
Copy link
Contributor Author

Johnlon commented Mar 29, 2024

Error

/home/runner/work/cloud-custodian/cloud-custodian/docs/source/aws/resources/appmesh-virtual-gateway.rst: WARNING: document isn't included in any toctree

Maybe I jave have to accept the old naming style and change the new one to match. @ajkerrigan

@ajkerrigan
Copy link
Member

Error

/home/runner/work/cloud-custodian/cloud-custodian/docs/source/aws/resources/appmesh-virtual-gateway.rst: WARNING: document isn't included in any toctree

Maybe I jave have to accept the old naming style and change the new one to match. @ajkerrigan

Hmm this is looking like a GitHub Actions cache-related failure... running make sphinx in a local dev environment from this branch works fine, and there's no lingering docs/source/aws/resources/appmesh-virtual-gateway.rst file to trigger the warning from the CI run.

But I see that our docs cache key comes from hashing dependency definitions (poetry.lock files). It may be more accurate to key that cache with hashes of .py files in each provider's resource directory 🤔 . This is a bigger discussion (not to be fixed as part of this PR, splitting this out into #9395), and in practice it's usually an academic concern since we're rebuilding docs during the build step as needed. Seems like these sorts of failures would only come up if we remove/rename a resource between CI runs before the docs cache expires.

So for this PR one ugly-but-functional way to go might be wait "a bit" (I don't know how long it takes) for the cache to expire and re-run CI. Not pretty, but I'm not sure of a better option.

@ajkerrigan
Copy link
Member

Oh huh, last I knew you couldn't delete these caches on demand but I guess you can now. Just deleted the most recent docs cache and retriggered the doc build.

Copy link
Member

@ajkerrigan ajkerrigan 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, thanks 👍

@ajkerrigan ajkerrigan merged commit 2fcf08f into cloud-custodian:main Mar 29, 2024
22 checks passed
@Johnlon
Copy link
Contributor Author

Johnlon commented Mar 29, 2024

Nice one thanks

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.

None yet

2 participants