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

fix: detect changes in dockerfile #6495

Merged
merged 4 commits into from
Mar 19, 2021
Merged

fix: detect changes in dockerfile #6495

merged 4 commits into from
Mar 19, 2021

Conversation

nikhname
Copy link
Contributor

Issue #, if available:
#6359
Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nikhname nikhname requested a review from a team as a code owner January 27, 2021 22:47
@nikhname nikhname removed the request for review from a team January 27, 2021 22:48
@codecov
Copy link

codecov bot commented Jan 27, 2021 •
edited
Loading

Codecov Report

Merging #6495 (1d547df) into master (e53175d) will decrease coverage by 0.55%.
The diff coverage is 15.00%.

❗ Current head 1d547df differs from pull request most recent head 5ffe22b. Consider uploading reports for the commit 5ffe22b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6495      +/-   ##
==========================================
- Coverage   56.52%   55.97%   -0.56%     
==========================================
  Files         430      433       +3     
  Lines       21150    21406     +256     
  Branches     4242     4294      +52     
==========================================
+ Hits        11955    11981      +26     
- Misses       8382     8612     +230     
  Partials      813      813
Impacted Files Coverage Δ
...der-awscloudformation/src/configuration-manager.ts 5.46% <ø> (ø)
.../extensions/amplify-helpers/update-amplify-meta.ts 15.50% <12.50%> (-0.50%) ⬇️
.../src/extensions/amplify-helpers/resource-status.ts 6.27% <16.66%> (ø)
...tensions/amplify-helpers/remove-pinpoint-policy.ts 35.29% <0.00%> (ø)
...xtensions/amplify-helpers/get-cloud-init-status.ts 46.15% <0.00%> (ø)
packages/amplify-cli/src/index.ts 42.38% <0.00%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b64844...5ffe22b. Read the comment docs.

@yuth yuth changed the title Fix: Detect changes in dockerfile fix: detect changes in dockerfile Feb 26, 2021
SwaySway
SwaySway approved these changes Mar 6, 2021
View reviewed changes
Copy link
Contributor

@SwaySway SwaySway left a comment

Choose a reason for hiding this comment

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

aside from the nits lgtm

packages/amplify-cli/src/extensions/amplify-helpers/update-amplify-meta.ts Outdated
@@ -61,7 +61,7 @@ function moveBackendResourcesToCurrentCloudBackend(resources: $TSObject[]) {
// in the case that the resource is being deleted, the sourceDir won't exist
if (fs.pathExistsSync(sourceDir)) {
fs.copySync(sourceDir, targetDir);
if (resource?.service === ServiceName.LambdaFunction) {
if (resource.service === ServiceName.LambdaFunction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason for removing the optional chain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this back, that was unintentional

packages/amplify-cli/src/extensions/amplify-helpers/update-amplify-meta.ts Outdated
function getHashForResourceDir(dirPath) {
const options = {
// This functions is also on resource-status, might need to be exported from somewhere else
function getHashForResourceDir(dirPath, files?: string[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we change this so both call the same function?

@nikhname nikhname linked an issue Mar 6, 2021 that may be closed by this pull request
Fargate hosting - updates / changes not registering as diff #6359
Closed
@nikhname nikhname force-pushed the dockerfileDetectChange branch from 921871a to 1d547df Compare March 8, 2021 20:59
@lgtm-com
Copy link

lgtm-com bot commented Mar 8, 2021

This pull request introduces 1 alert when merging 1d547df into dc1ebcd - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@ammarkarachi ammarkarachi merged commit 2333dec into aws-amplify:master Mar 19, 2021
@lgtm-com
Copy link

lgtm-com bot commented Mar 19, 2021

This pull request introduces 1 alert when merging 5ffe22b into 3b64844 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@jhockett jhockett mentioned this pull request Jun 11, 2021
Closed
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SwaySway SwaySway SwaySway approved these changes

@edwardfoyle edwardfoyle Awaiting requested review from edwardfoyle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fargate hosting - updates / changes not registering as diff
4 participants
@nikhname @SwaySway @ammarkarachi @elorzafe