Skip to content

Conversation

@sonamkindy
Copy link
Contributor

@sonamkindy sonamkindy commented Oct 14, 2022

Problem

masking and containerized masking package builds need to retrieve secrets from AWS.

This is especially critical for when DLPX-81645 Remove secrets from tip of dms-core-gate repo changes are integrated.

Testing

→ git-ab-pre-push -b "masking" --jenkins-url=http://selfservice.pkg-test.dcol2.delphix.com
Your build is at: http://selfservice.pkg-test.dcol2.delphix.com/job/appliance-build-orchestrator-pre-push/7/

Per http://selfservice.pkg-test.dcol2.delphix.com/job/linux-pkg/job/6.0/job/stage/job/build-package/job/masking/job/pre-push/5/console:

13:25:13  Success: Package masking has been built successfully.

Future Improvements

See GHM-833.

@sonamkindy sonamkindy force-pushed the dlpx/pr/sonamkindy/99fc7416-8448-4f31-8dcb-1c66aeda8567 branch 2 times, most recently from 16049ec to fa2d6c6 Compare October 14, 2022 02:09
@sonamkindy sonamkindy requested review from a user, grodr, prakashsurya and robinrye October 14, 2022 02:13
@sonamkindy sonamkindy marked this pull request as ready for review October 14, 2022 15:55
Copy link
Contributor

@prakashsurya prakashsurya left a comment

Choose a reason for hiding this comment

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

Seems reasonable.. but, could we be consistent with #256.. it seems like both are doing the same thing, but with slightly different implementations.. even better, would probably be to move this logic into a function within the common.sh file, and have both call the common function..

I don't feel strongly, so I'm OK moving forward as-is (hence my approval), but I do think it'd be nice to consolidate the two and be consistent with each other (as much as would be reasonable, at least)..

Even in this one review, we have the duplication.. so having the common function might help us maintain this moving forward..

@sonamkindy
Copy link
Contributor Author

sonamkindy commented Oct 14, 2022

Seems reasonable.. but, could we be consistent with #256.. it seems like both are doing the same thing, but with slightly different implementations.. even better, would probably be to move this logic into a function within the common.sh file, and have both call the common function..

I don't feel strongly, so I'm OK moving forward as-is (hence my approval), but I do think it'd be nice to consolidate the two and be consistent with each other (as much as would be reasonable, at least)..

Even in this one review, we have the duplication.. so having the common function might help us maintain this moving forward..

Sure thing, I can definitely consolidate that logic in a function in common.sh. The one discrepancy between the virtualization build config and masking/containerized masking build configs is that the former requires them as ant parameters whereas the latter requires them as environment variables. Are you amenable to me having a function just export the variables as environment variables? Since it's duplicated in both masking/containerized masking config shell scripts, it seems like it'd be the most useful to have them implement all of the necessary functionality.

FYI: raised GHM-833 per slack convo with @prakashsurya.

@skinner-m-c
Copy link
Contributor

Might want to consider explicitly including the environment variables for the command. Exporting sends these variables to every command (e.g. the following rsync and cp commands and anything else added after this in the future).

@sonamkindy sonamkindy force-pushed the dlpx/pr/sonamkindy/99fc7416-8448-4f31-8dcb-1c66aeda8567 branch from fa2d6c6 to 65aab7b Compare October 20, 2022 19:26
@sonamkindy sonamkindy force-pushed the dlpx/pr/sonamkindy/99fc7416-8448-4f31-8dcb-1c66aeda8567 branch from 65aab7b to 7088a99 Compare October 26, 2022 16:46
@sonamkindy sonamkindy force-pushed the dlpx/pr/sonamkindy/99fc7416-8448-4f31-8dcb-1c66aeda8567 branch from 7088a99 to 234e301 Compare October 26, 2022 16:46
@sonamkindy sonamkindy merged commit 89d357e into 6.0/stage Oct 26, 2022
@sonamkindy sonamkindy deleted the dlpx/pr/sonamkindy/99fc7416-8448-4f31-8dcb-1c66aeda8567 branch October 26, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants