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

feat: allow skipping aws s3 uploads #48

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: allow skipping aws s3 uploads #48

wants to merge 2 commits into from

Conversation

fmartingr
Copy link
Contributor

Summary

Allows plugins to skip the AWS S3 artifact upload by using a new input argument: skip-s3-upload

@fmartingr fmartingr requested a review from a team as a code owner September 2, 2024 12:13
@@ -29,6 +29,13 @@ on:
required: false
type: string

skip-aws-upload:
Copy link
Contributor

Choose a reason for hiding this comment

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

For the CD part it's imperative to upload. This is how we download the pre-packaged plugins .
Do we also need the flag there ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we get the release on Github but not uploaded to S3, since the bundles are attached to releases here there's no need to have the S3 files around.

Copy link
Contributor

Choose a reason for hiding this comment

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

When you bundle a plugin with mattermost platform currently it is being fetched from the S3 bucket through a cloudfront distribution .
Since we have not yet changed this functionality we should continue uploading artifacts to S3
https://github.com/mattermost/mattermost/blob/master/server/build/release.mk#L147

If you are completely sure that your plugin is not going to be bundled in the future with mattermost platform then you should use this but I suggest against that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case this plugin won't be bundled since it will become EoL the moment the full feature arrives in Mattermost, and I see this being like that for a number of plugins: cc: @wiggin77

Are we putting plugins in the bucket "just in case", are we doing any kind of cleanup of deprecated/deleted/outdated plugins?

Copy link
Member

@saturninoabril saturninoabril Sep 2, 2024

Choose a reason for hiding this comment

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

@fmartingr Thanks for taking the time to look at this. I made a comment in our internal discussion to clarify that we're not saving "just in case" but as main data store - https://hub.mattermost.com/private-core/pl/epzwcuorgjgpzxgrnjamfqhrcc

are we doing any kind of cleanup of deprecated/deleted/outdated plugins?

No, for it's protected (data integrity, compliance, audit, etc) but can do so if requested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. Removed the option from CD.

Copy link
Contributor

@toninis toninis left a comment

Choose a reason for hiding this comment

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

I will get back to you

@@ -100,7 +107,7 @@ jobs:
uses: mattermost/actions/plugin-ci/build@cd879ea9c64cc3e26a75a042d1c5066be28130a6

delivery:
if: ${{ github.repository_owner == 'mattermost' && github.event_name != 'schedule' && (github.ref_name == 'master' || github.ref_name == 'main') }}
if: ${{ github.repository_owner == 'mattermost' && github.event_name != 'schedule' && (github.ref_name == 'master' || github.ref_name == 'main') && !inputs.skip-aws-upload }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I will have to test that . I do not think this is evaluated as boolean .
Check here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I forgot about that, but you're right, we need to use fromJSON or use string comparison.

@fmartingr fmartingr requested a review from toninis November 21, 2024 14:37
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.

3 participants