Skip to content

S3 for Public Feed Downloads#652

Open
binh-dam-ibigroup wants to merge 18 commits into
devfrom
use-s3-for-public-feed-downloads
Open

S3 for Public Feed Downloads#652
binh-dam-ibigroup wants to merge 18 commits into
devfrom
use-s3-for-public-feed-downloads

Conversation

@binh-dam-ibigroup

@binh-dam-ibigroup binh-dam-ibigroup commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

This PR adds support for the use of S3 bucket for public feed hosting, even if S3 is not used for storing feed versions for internal DataTools use.

Behind the scenes, support is added for alternate S3 credentials, region, and bucket for the MTCFeedResource and FeedUpdater that, if specified, override the application's ones:

extensions
  mtc
    s3_region
    s3_bucket
    s3_credentials_file

@br648 br648 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some minor nits. Also, this might be a good time to migrate to the newer and latest versions of Amazon AWS dependencies e.g. com.amazonaws -> software.amazon.awssdk

s3client = builder.build();
} catch (Exception e) {
LOG.error(
"S3 client not initialized correctly. Must provide config property {} or specify region in ~/.aws/config",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit. Remove additional space before Must

/**
* Implements the default behavior for above interface.
* This method uses extensions.mtc.s3_credentials_file and s3_region, if populated,
* and falls back to .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Complete sentence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that slipped, thanks (b3a9f2e)

url = S3Utils.getDefaultBucketUrlForKey(fs.toPublicKey());
}
r.append("<li>");
r.append("<a href=\"" + url + "\">");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider using the appender instead of + : r.append("<a href=\"").append(url).append("\">");. Same for lines 94 and 97.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated in 7f138f0 although I wasn't initially planning to touch that.

}

@ParameterizedTest
@ValueSource(booleans = { false, true })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is cool!

@br648 br648 assigned binh-dam-ibigroup and unassigned br648 Jun 5, 2026
@binh-dam-ibigroup

Copy link
Copy Markdown
Contributor Author

Also, this might be a good time to migrate to the newer and latest versions of Amazon AWS dependencies

We have an internal backlog ticket for that!

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