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

Send staging reason to SQS queue #2835

Merged
merged 2 commits into from
Jul 26, 2019

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Jul 22, 2019

Motivation

In #2789 (comment), @techman83 pointed out that the SQS queue code in #2798 does not handle the x_netkan_staging_reason property from KSP-CKAN/NetKAN-bot#88.

Changes

Now the x_netkan_staging_reason property flows through into a Metadata.StagingReason property, which will be passed to a new message attribute if set:

Attr StagingReason: The reason that staging is enabled for this module (if it is enabled), to help pull request reviewers determine whether to merge

Notes:

  • If the module is staged but x_netkan_staging_reason is not set, the attribute will not be set
  • If the module is not staged but x_netkan_staging_reason is set, the attribute will not be set

So the downstream code should be prepared to handle a staged module without this attribute.

x_netkan_staging and x_netkan_staging_reason are added to the spec (according to their current behavior).

@HebaruSan HebaruSan added Enhancement New features or functionality Easy This is easy to fix Pull request Netkan Issues affecting the netkan data labels Jul 22, 2019
@HebaruSan HebaruSan requested a review from techman83 July 22, 2019 20:20
@HebaruSan HebaruSan added the In progress We're still working on this label Jul 22, 2019
@HebaruSan HebaruSan added Spec Issues affecting the spec and removed In progress We're still working on this labels Jul 22, 2019
@techman83
Copy link
Member

My only issue here is, does it belong in the resulting metadata? But that's from a purist standpoint and I'm ok if this is the path of least resistance to get it done.

Also from what I undestand x_blah things are ignored by the client, so does it need to be in the spec?

@HebaruSan
Copy link
Member Author

My only issue here is, does it belong in the resulting metadata?

I thought the goal was to get it into the body of the pull request. Strings like "Check that the version matches the forum thread" would not be useful to have in CKAN-meta.

@HebaruSan
Copy link
Member Author

Also from what I undestand x_blah things are ignored by the client, so does it need to be in the spec?

Spec.md has a section for netkan syntax, including several x_netkan_* properties.

https://github.com/KSP-CKAN/CKAN/blob/master/Spec.md#netkan-fields

I think it makes sense to document the staging properties so authors and metadata maintainers can make use of them as needed.

@techman83
Copy link
Member

techman83 commented Jul 26, 2019

My only issue here is, does it belong in the resulting metadata?

I thought the goal was to get it into the body of the pull request. Strings like "Check that the version matches the forum thread" would not be useful to have in CKAN-meta.

Ohhhhh, I might definitely have misunderstood! Been a very long week 😴 (for reference, I thought this flowing all the way through, not just going to SQS)

I'm also not as familiar with the inner workings of NetKAN as I would like 🙂

Thanks for your efforts!

@techman83 techman83 merged commit 2a6a54f into KSP-CKAN:master Jul 26, 2019
@HebaruSan HebaruSan deleted the feature/netkan-staging-reason branch July 26, 2019 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy This is easy to fix Enhancement New features or functionality Netkan Issues affecting the netkan data Spec Issues affecting the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants