Skip to content
This repository has been archived by the owner on Oct 13, 2023. It is now read-only.

[ART-7273] add go wrapper script in doozer #811

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

Ximinhan
Copy link
Contributor

@Ximinhan Ximinhan commented Aug 2, 2023

add go wrapper script in doozerlib path named golang_builder_FIPS_wrapper.sh
extend add modification method to support using local path from doozer source instead of only support URL source
the new key under add modification is doozer_source witch take relative source path within doozer

@joepvd
Copy link
Contributor

joepvd commented Aug 10, 2023

/jira refresh

@@ -79,12 +79,14 @@ def __init__(self, *args, **kwargs):
:param path: Destination path to the dist-git repo.
:param overwriting: True to allow to overwrite if path exists.
Setting to false to prevent from accidently overwriting files from in-tree source.
:param doozersource: Destination path to the doozer source
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we split words with _ if an attribute has multiple words.

Copy link
Contributor

Choose a reason for hiding this comment

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

Examplation sounds wrong. Maybe Source path local to doozer

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this attribute is not used in openshift-eng/ocp-build-data#3356

@@ -79,12 +79,14 @@ def __init__(self, *args, **kwargs):
:param path: Destination path to the dist-git repo.
:param overwriting: True to allow to overwrite if path exists.
Setting to false to prevent from accidently overwriting files from in-tree source.
:param doozersource: Destination path to the doozer source
Copy link
Contributor

Choose a reason for hiding this comment

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

Examplation sounds wrong. Maybe Source path local to doozer

if ceiling_dir and not is_in_directory(path, ceiling_dir):
raise ValueError("Writing to a file out of {} is not allowed.".format(ceiling_dir))
# NOTE: `overwriting` is checked before writing.
dest_address = str(kwargs["context"]['distgit_path'].joinpath(self.path))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not an address, but a path. I think?

@@ -0,0 +1,344 @@
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not really like the file name, for these reasons:

  • in config, we need: source: target_go_wrapper.sh. Think the word target will cause confusions
  • everyone speaks of this as fips_or_die. So would be good to have that in the name. Maybe fips_or_die_wrapper?

raise ValueError("Unknown 'validate' value: {self.validate}")
else: # use doozer local source
content = Path(Path(__file__).parent, self.doozersource).read_text()
source_address = self.doozersource
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not an address. Think the variable is also not strictly necessary.

@@ -79,12 +79,14 @@ def __init__(self, *args, **kwargs):
:param path: Destination path to the dist-git repo.
:param overwriting: True to allow to overwrite if path exists.
Setting to false to prevent from accidently overwriting files from in-tree source.
:param doozersource: Destination path to the doozer source
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this attribute is not used in openshift-eng/ocp-build-data#3356

Copy link
Contributor

@joepvd joepvd left a comment

Choose a reason for hiding this comment

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

/lgtm

Let's try it :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants